Prepare revrem for public launch#6
Merged
Merged
Conversation
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request establishes the public launch baseline for code-review-loop, introducing core features such as a TOML-based profile system, append-only run history, and an optional Textual TUI. It also includes comprehensive documentation, GitHub community files, and a robust test suite. Review feedback focuses on enhancing the reliability of file operations, specifically by preserving permissions during atomic writes, preventing file descriptor leaks, and implementing synchronization for shared history logs. Additionally, suggestions were made to optimize memory usage during subprocess execution and to adopt a dedicated library for TOML generation to improve long-term maintainability.
|
|
||
| def _atomic_write_text(path: Path, content: str) -> None: | ||
| path.parent.mkdir(parents=True, exist_ok=True) | ||
| fd, tmp_name = tempfile.mkstemp(dir=path.parent, prefix=f".{path.name}.", suffix=".tmp") |
There was a problem hiding this comment.
The use of tempfile.mkstemp creates a file with 0600 permissions by default. When this temporary file replaces the target configuration file at line 853, any existing permissions on the original file (e.g., 0644 for a project-level config) will be lost. This can break shared access to configuration files in collaborative environments. Consider using shutil.copymode to preserve the original file's permissions if it exists.
| tmp_path = Path(tmp_name) | ||
| try: | ||
| with os.fdopen(fd, "w", encoding="utf-8") as handle: | ||
| handle.write(content) |
There was a problem hiding this comment.
If os.fdopen fails (for example, due to an invalid encoding or system error), the file descriptor fd returned by mkstemp will leak until the process terminates. While unlikely in this context, it is safer to wrap the fdopen call in a try...except block or ensure the descriptor is closed if the stream cannot be created.
Comment on lines
+62
to
+63
| record = history_record(summary, cwd=cwd) | ||
| with target.open("a", encoding="utf-8") as handle: |
There was a problem hiding this comment.
Appending to the history file is not synchronized. If multiple instances of the utility are run concurrently (e.g., in different terminal tabs or by automated scripts), the JSONL records could interleave or become corrupted. Although this is a local tool, using a file lock (e.g., via fcntl on POSIX) would ensure the integrity of the shared history file.
| capture_output: bool = True, | ||
| ) -> subprocess.CompletedProcess[str]: | ||
| argv = current_entrypoint_argv(plan.argv) | ||
| return subprocess.run( |
There was a problem hiding this comment.
Using subprocess.run with capture_output=True buffers the entire stdout and stderr of the child process in memory. If a review or remediation pass generates an exceptionally large amount of output, this could lead to high memory consumption. For long-running processes, consider streaming the output or using a temporary file if the TUI needs to display it later.
Comment on lines
+500
to
+542
| def _profile_to_toml_impl( | ||
| profile: Profile, | ||
| *, | ||
| root: tuple[str, ...] | None, | ||
| omit_builtin_defaults: bool, | ||
| omit_reference_defaults: bool = False, | ||
| reference: Profile | None = None, | ||
| raw_profile: dict[str, Any] | None = None, | ||
| ) -> str: | ||
| lines: list[str] = [] | ||
| if root is not None: | ||
| lines.append(f"[{_toml_table_header(root)}]") | ||
| if profile.description: | ||
| lines.append(f"description = {_toml_string(profile.description)}") | ||
| lines.append("") | ||
| elif profile.description: | ||
| lines.append(f"description = {_toml_string(profile.description)}") | ||
| lines.append("") | ||
| for section_name in ("pipeline", "review", "triage", "remediation", "commit", "output", "runtime"): | ||
| value = getattr(profile, section_name) | ||
| header = (*root, section_name) if root is not None else (section_name,) | ||
| section_lines: list[str] = [] | ||
| defaults = type(value)() | ||
| reference_value = getattr(reference, section_name) if reference is not None else None | ||
| raw_section = raw_profile.get(section_name) if raw_profile is not None else None | ||
| for key, item in asdict(value).items(): | ||
| if item is None: | ||
| continue | ||
| # Preserve explicit profile overrides even when they match a built-in default. | ||
| # File-level defaults must win first so saved/cloned profiles reload identically. | ||
| reference_item = getattr(reference_value, key) if reference_value is not None else None | ||
| if omit_reference_defaults and reference_value is not None and item == reference_item: | ||
| continue | ||
| explicit = isinstance(raw_section, dict) and key in raw_section | ||
| if omit_builtin_defaults and item == getattr(defaults, key) and not explicit: | ||
| continue | ||
| section_lines.append(f"{key} = {_toml_value(item)}") | ||
| if not section_lines: | ||
| continue | ||
| lines.append(f"[{_toml_table_header(header)}]") | ||
| lines.extend(section_lines) | ||
| lines.append("") | ||
| return "\n".join(lines).rstrip() + "\n" |
There was a problem hiding this comment.
The manual TOML generation logic in _profile_to_toml_impl is functional for the current schema but may become fragile as the configuration complexity grows. It lacks handling for complex escaping scenarios or nested structures beyond the current depth. Consider using a dedicated TOML writing library (like tomli-w) to ensure full compliance with the TOML specification and better maintainability.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (5)
scripts/dev-check (1)
13-19: ⚡ Quick winSystem
mypybranch bypasses the venv interpreter.When the
ifbranch on line 13 fires,mypy srcruns with the system mypy rather than the venv's mypy. This can produce stale or mismatched results if the venv contains type stubs or a different mypy version. Theelifbranch already uses the correct"$PYTHON" -m mypyform — theifbranch should match it.♻️ Proposed fix
-if command -v mypy >/dev/null 2>&1; then - mypy src -elif "$PYTHON" -m mypy --version >/dev/null 2>&1; then +if "$PYTHON" -m mypy --version >/dev/null 2>&1; then "$PYTHON" -m mypy src else printf '%s\n' 'mypy not installed; skipping type check' >&2 fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/dev-check` around lines 13 - 19, The current if-branch uses the system mypy ("mypy src") which bypasses the venv; change the script so both branches invoke mypy via the venv interpreter (use "$PYTHON" -m mypy src) instead of running the bare "mypy" command. Update the conditional that currently checks `if command -v mypy >/dev/null 2>&1; then` so it calls `"$PYTHON" -m mypy src` (matching the `elif` branch), ensuring any invocation of mypy uses the PYTHON interpreter variable used elsewhere in this script.docs/00-governance/templates/prd.template.md (1)
2-23: ⚡ Quick winAdd an explicit immutability note for
document_idin the template.Please add a short instruction that
document_idis write-once after creation to prevent accidental edits during updates.Suggested change
> **Document ID:** {{document_id}} +> **Note:** `document_id` is immutable after initial assignment. > **Owner:** {{owner}}Based on learnings: "Never modify a
document_idonce set".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/00-governance/templates/prd.template.md` around lines 2 - 23, Add a short immutability instruction stating that document_id is write-once after creation to prevent accidental edits: insert a one-line note near the metadata block (e.g., alongside the YAML frontmatter or immediately above/below <!-- MEMINIT_METADATA_BLOCK -->) referencing the document_id field and saying it must not be modified after initial set; ensure the note appears both in the YAML frontmatter (document_id: {{document_id}}) area and in the human-readable metadata block so readers and tooling see the write-once constraint.tests/test_harnesses.py (1)
49-59: ⚡ Quick winStrengthen remediation command assertions for sandbox/color value wiring.
You verify flag presence/order, but not the actual values for
--sandboxand--color; adding those assertions would catch more real regressions.Suggested change
assert "--json" in command + assert command[command.index("--sandbox") + 1] == "workspace-write" + assert command[command.index("--color") + 1] == "never" assert command[command.index("--model") + 1] == "gpt-5.4-mini" assert command[-3:] == ["--output-last-message", "last.txt", "-"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_harnesses.py` around lines 49 - 59, The test currently checks presence/order of flags but not the actual values wired for sandbox and color; update tests in tests/test_harnesses.py to assert that the token immediately after "--sandbox" equals the expected sandbox value for this test and that the token immediately after "--color" equals the expected color value (i.e., assert command[command.index("--sandbox")+1] == <expected_sandbox_value> and command[command.index("--color")+1] == <expected_color_value>), so any regressions in sandbox/color wiring are caught.docs/05-planning/tasks/task-001-public-github-launch-readiness.md (1)
553-622: Release workflow fires on tag push without an inline test gate.Task 10 defines the CI workflow, but the release workflow (
.github/workflows/release.yml, referenced in the PR) triggers onv*.*.*tag pushes without requiring the CI checks to have passed first. This means a tag pushed against a broken commit can trigger a public artifact build and publish cycle without test coverage.Recommended fix: add a
needs: [ci]dependency in the release workflow job, or require a passing CI run as a prerequisite for the release job (e.g. viaworkflow_runtrigger or GitHub environment protection rules). This aligns with the task 10 intent of CI being green before release jobs are added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/05-planning/tasks/task-001-public-github-launch-readiness.md` around lines 553 - 622, The release workflow (.github/workflows/release.yml) currently triggers on v*.*.* tag pushes without ensuring CI passed; update the release workflow so its release job depends on the CI job (e.g., add needs: [ci] on the release job or reference the CI workflow via workflow_run) or require a protected GitHub environment with successful CI as a prerequisite; target the release job name (e.g., "release" or "publish") in your change so the workflow will only proceed after the CI workflow completes successfully.tests/test_packaging.py (1)
134-210: 💤 Low valueClarify the assertion on line 208.
The assertion
assert "exit 0" in stale_python.read_text(...)appears to verify that the stale venv's Python was overwritten by the new fake venv creation. However, this relies on the specific content of the fake python script created by the test harness. Consider adding a brief comment explaining this expectation for future maintainers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_packaging.py` around lines 134 - 210, The assertion in test_promote_stable_recreates_stale_stable_venv that checks assert "exit 0" in stale_python.read_text(...) is unclear to future readers; add a short inline comment above this assertion stating that the test harness's fake_python/venv creation writes a script containing "exit 0", so verifying that string confirms the stale venv's python was overwritten by the new fake venv (referencing the fake_python and stale_python variables and the test function name to locate the lines).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/meminit-docops/SKILL.md:
- Around line 308-309: The spec reference mismatch: update the doc so the
normative contract and the linked spec match by either changing the text that
references MEMINIT-SPEC-008 (v3) or replacing the link at line 342 that
currently points to the v2 file; ensure the document consistently references
MEMINIT-SPEC-008 v3 (replace any `...-v2.md` link with the v3 equivalent) and
update any stray reference to MEMINIT-SPEC-004 to MEMINIT-SPEC-008 where
intended.
In @.github/ISSUE_TEMPLATE/config.yml:
- Around line 4-7: The URLs in the "url" fields for security advisories and
support policy are pointing to the hardcoded repository
"GitCmurf/code-review-loop" and should be updated to point to the current
repository dynamically or use the appropriate repository name to avoid
misrouting reports and support requests. Modify these URLs in the config.yml so
they reference the current repo's paths correctly rather than hardcoded external
paths.
In @.github/workflows/ci.yml:
- Around line 3-6: The release workflow can be triggered by v*.*.* tag pushes
without running CI tests first; update the release workflow's publish job (the
job named "publish" or equivalent) to require the test matrix by adding needs:
test (or reference the test job name used in the release workflow) so the
publish job depends on the test job completing successfully, or alternatively
enforce in the release workflow that tags are only allowed after successful CI
on main; modify the release workflow to include needs: test for the publish job
to ensure tests run before publishing.
- Around line 3-6: The release workflow's publish job is missing a dependency on
the test job so tag pushes can bypass CI; update the publish job (job name
"publish") to include needs: test (or the actual test job id, e.g., "test") so
the publish step waits for the matrix/test job to succeed before publishing on
v*.*.* tag triggers; ensure the publish job's needs array references the
existing test job identifier used elsewhere in the workflow.
- Line 33: Update the CI pip install step that installs the git-pinned meminit
dependency by adding pip's hash-checking mode: include the --require-hashes flag
on the python -m pip install "meminit @
git+https://github.com/GitCmurf/meminit.git@63547bc79f46200d25e4b7375b5c661f64aa34f8"
invocation so pip enforces hash verification for the pinned commit; ensure the
command and any generated requirements file include matching hashes so the
install succeeds under --require-hashes.
In @.github/workflows/release.yml:
- Around line 13-57: Add a release-time verification gate by inserting a
blocking verification step before the "Publish GitHub release" step in the build
job: run the project's full verification suite (e.g., a step named "Verify
artifacts" that runs linting, unit/integration tests like python -m pytest and
any packaging checks) so the job fails on bad artifacts, and/or require a
protected environment approval by adding environment protection to the publish
step (referencing the existing "Publish GitHub release" step) so publishing is
blocked until manual approvers approve; ensure the new step runs after "Upload
workflow artifact" and before "Publish GitHub release" and aborts the workflow
on non-zero exit so bad tags cannot publish.
In `@CONTRIBUTING.md`:
- Around line 11-13: Replace the stale repository slug used in contributor
setup/verification commands: find occurrences of the string
"GitCmurf/code-review-loop" (e.g., in the git clone examples around the lines
showing "git clone https://github.com/GitCmurf/code-review-loop.git" and the
nearby commands) and update them to the correct repository slug
"GitCmurf/revrem" so the clone and related copy-paste instructions work for
contributors; verify you update every occurrence (notably the blocks around
lines ~11 and ~101-103) and run a quick read-through to ensure no other stale
slugs remain.
In `@docs/05-planning/plan-001-greenfield-docops-bootstrap-exercise.md`:
- Line 94: Remove the hard-coded absolute path "/home/cmf/code/code-review-loop"
and replace it with a generic, non-identifying phrase such as "the final
repository exists in your local development directory (e.g., <project-root>)" or
"the final repository exists in the user's local workspace" so no developer
username or machine layout is exposed; update the line in
docs/05-planning/plan-001-greenfield-docops-bootstrap-exercise.md by finding the
exact string "/home/cmf/code/code-review-loop" and substituting the generic
wording.
In `@docs/05-planning/tasks/task-001-public-github-launch-readiness.md`:
- Around line 117-127: The three fenced code blocks for the task dependency
graph, the NOTICE attribution, and the CODEOWNERS pattern are missing language
specifiers (MD040); update each opening fence to include a language token—use
```text for the plain-text blocks (task dependency graph and NOTICE attribution)
and either ```text or ```gitconfig for the CODEOWNERS pattern—so the three
blocks (the dependency graph block shown with arrows, the NOTICE/copyright
block, and the CODEOWNERS pattern block) each start with a fenced code fence
that includes the appropriate language specifier.
In
`@docs/10-prd/prd-001-interactive-tui-and-profile-system-for-code-review-loop.md`:
- Around line 298-300: Fix MD038 spacing inside inline code spans by removing
the leading space in the backtick-wrapped revision tag: replace occurrences of
the literal code span "` (RevRem)`" with "`(RevRem)`" (also check and update the
same pattern around the nearby lines where "` (RevRem)`" appears, e.g., the
region containing the text "`(RevRem)`" references).
In `@docs/58-logs/log-001-public-launch-audit.md`:
- Around line 56-63: Update the "Secret And PII Scan Evidence" section to
explicitly address the `.superpowers/` directory: inspect any committed files
under `.superpowers/` and state whether they contain local absolute
paths/usernames (and list examples if found) or confirm they are clean; if
artifacts were removed, state the removal method (e.g. `git filter-repo`) and
confirm the directory was added to `.gitignore`; update the relevant table rows
(the "Targeted working-tree text search" and the history scan row) to reflect
which of the two actions was taken and include the date/commit or command used
so reviewers can verify.
In `@docs/70-devex/devex-001-using-code-review-loop.md`:
- Around line 397-399: The markdown code span for the commit subject currently
contains a leading space: the token `` ` (RevRem)` ``; update it to remove the
leading space so it reads `` `(RevRem)` `` to satisfy markdownlint MD038
(no-space-in-code). Locate the sentence mentioning the default subject policy
and `--commit-message-prompt` and replace the code span token accordingly.
In `@README.md`:
- Around line 54-56: The README install block uses a stale clone URL ("git clone
https://github.com/GitCmurf/code-review-loop.git"); update that git clone
command to point at the public launch repository (e.g., "git clone
https://github.com/openai/code-review-loop.git") so first-run setup works; edit
the git clone line in the install block of README.md to replace the old
owner/repo with the public launch repo and leave the subsequent cd and
./scripts/install-dev lines unchanged.
In `@scripts/dev-python`:
- Line 8: The script currently unconditionally runs exec python3 "$@" which can
pick up older Python interpreters; update the startup logic in
scripts/dev-python to explicitly prefer python3.11: try to locate and exec
python3.11 first, fall back to python3 only if python3.11 is unavailable, and
before exec verify the chosen interpreter reports sys.version_info >= (3,11) (or
print a clear error and exit non‑zero). Ensure the change replaces the
single-line exec python3 "$@" invocation and uses a short runtime check (e.g.,
invoking the candidate interpreter with a small -c snippet) so callers always
run on Python >=3.11.
In `@scripts/promote-stable`:
- Around line 53-55: The script currently does an unconditional rm -rf
"$STABLE_VENV" which can delete dangerous locations if STABLE_VENV is empty or
set to root-equivalent values; before the remove in the block that checks
stable_venv_needs_rebuild, add a guard that verifies STABLE_VENV is non-empty
and not one of unsafe values (e.g. "/" or "." or empty) and optionally ensure it
matches an expected pattern (for example starts with the repo path or contains
"venv" substring); if the check fails, print an error and exit non‑zero instead
of running rm -rf; reference the STABLE_VENV, stable_venv_needs_rebuild, and
PYTHON variables and perform the guard immediately before rm -rf "$STABLE_VENV".
In `@SECURITY.md`:
- Line 13: The SECURITY.md file contains a hard-coded private advisory URL
`https://github.com/GitCmurf/code-review-loop/security/advisories/new` that
points to the wrong repository; update that URL to use this repository's correct
GitHub path (replace `GitCmurf/code-review-loop` with the current repo
owner/name) so private vulnerability intake opens on the right repo and verify
the revised link format matches GitHub's advisory URL pattern.
In `@src/code_review_loop/harnesses.py`:
- Around line 160-163: The _codex_config_args function currently interpolates
reasoning_effort directly into the '-c' argument, which breaks if the value
contains quotes or other unsafe chars; validate that reasoning_effort matches a
restricted pattern (e.g. only alphanumerics and hyphens) before building the
argument: use a regex full-match (e.g. r'^[A-Za-z0-9-]+$') to accept the value,
and if it fails validation either return an empty list or raise a ValueError
(choose consistent behavior with the module) instead of interpolating the raw
string; update _codex_config_args to perform this check and only produce ["-c",
f'model_reasoning_effort="{reasoning_effort}"'] when the validation passes.
In `@tests/test_devex_doc.py`:
- Around line 17-29: The test currently assumes history_rows has at least one
entry and that the front_matter version string uses single quotes, which can
raise IndexError or false negatives; update the test to first assert
history_rows is non-empty with a clear failure message (so the test fails
meaningfully if the Version History table is missing), compute latest_row =
history_rows[0] only after that guard, extract latest_version from latest_row
and normalize both the extracted latest_version and the version_line value to
semantic versions (e.g., strip surrounding quotes and whitespace) before
comparing, and assert equality of the normalized versions rather than string
equality (use the variables version_line, history_rows, latest_row,
latest_version, and front_matter to locate the code to change).
In `@tests/test_run_history.py`:
- Around line 8-10: In test_append_and_read_history_round_trip, clear the
XDG_DATA_HOME environment variable before configuring HOME so the expected path
is deterministic: call monkeypatch.delenv("XDG_DATA_HOME", raising=False) (or
monkeypatch.setenv("XDG_DATA_HOME", "") if you prefer) at the top of the test
(before monkeypatch.setenv("HOME", ...)) to ensure the test does not pick up the
runner's XDG_DATA_HOME.
---
Nitpick comments:
In `@docs/00-governance/templates/prd.template.md`:
- Around line 2-23: Add a short immutability instruction stating that
document_id is write-once after creation to prevent accidental edits: insert a
one-line note near the metadata block (e.g., alongside the YAML frontmatter or
immediately above/below <!-- MEMINIT_METADATA_BLOCK -->) referencing the
document_id field and saying it must not be modified after initial set; ensure
the note appears both in the YAML frontmatter (document_id: {{document_id}})
area and in the human-readable metadata block so readers and tooling see the
write-once constraint.
In `@docs/05-planning/tasks/task-001-public-github-launch-readiness.md`:
- Around line 553-622: The release workflow (.github/workflows/release.yml)
currently triggers on v*.*.* tag pushes without ensuring CI passed; update the
release workflow so its release job depends on the CI job (e.g., add needs: [ci]
on the release job or reference the CI workflow via workflow_run) or require a
protected GitHub environment with successful CI as a prerequisite; target the
release job name (e.g., "release" or "publish") in your change so the workflow
will only proceed after the CI workflow completes successfully.
In `@scripts/dev-check`:
- Around line 13-19: The current if-branch uses the system mypy ("mypy src")
which bypasses the venv; change the script so both branches invoke mypy via the
venv interpreter (use "$PYTHON" -m mypy src) instead of running the bare "mypy"
command. Update the conditional that currently checks `if command -v mypy
>/dev/null 2>&1; then` so it calls `"$PYTHON" -m mypy src` (matching the `elif`
branch), ensuring any invocation of mypy uses the PYTHON interpreter variable
used elsewhere in this script.
In `@tests/test_harnesses.py`:
- Around line 49-59: The test currently checks presence/order of flags but not
the actual values wired for sandbox and color; update tests in
tests/test_harnesses.py to assert that the token immediately after "--sandbox"
equals the expected sandbox value for this test and that the token immediately
after "--color" equals the expected color value (i.e., assert
command[command.index("--sandbox")+1] == <expected_sandbox_value> and
command[command.index("--color")+1] == <expected_color_value>), so any
regressions in sandbox/color wiring are caught.
In `@tests/test_packaging.py`:
- Around line 134-210: The assertion in
test_promote_stable_recreates_stale_stable_venv that checks assert "exit 0" in
stale_python.read_text(...) is unclear to future readers; add a short inline
comment above this assertion stating that the test harness's fake_python/venv
creation writes a script containing "exit 0", so verifying that string confirms
the stale venv's python was overwritten by the new fake venv (referencing the
fake_python and stale_python variables and the test function name to locate the
lines).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da0edd4b-7832-46ad-b31d-11d5e45e40bb
⛔ Files ignored due to path filters (8)
docs/05-planning/.meminit.lockis excluded by!**/*.lockdocs/05-planning/tasks/.meminit.lockis excluded by!**/*.lockdocs/10-prd/.meminit.lockis excluded by!**/*.lockdocs/45-adr/.meminit.lockis excluded by!**/*.lockdocs/55-testing/.meminit.lockis excluded by!**/*.lockdocs/58-logs/.meminit.lockis excluded by!**/*.lockdocs/70-devex/.meminit.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (63)
.agents/skills/meminit-docops/SKILL.md.agents/skills/meminit-docops/scripts/meminit_brownfield_plan.sh.editorconfig.env.example.github/CODEOWNERS.github/ISSUE_TEMPLATE/bug_report.yml.github/ISSUE_TEMPLATE/config.yml.github/ISSUE_TEMPLATE/docs.yml.github/ISSUE_TEMPLATE/feature_request.yml.github/dependabot.yml.github/pull_request_template.md.github/workflows/ci.yml.github/workflows/release.yml.github/workflows/scorecard.yml.gitignore.pre-commit-config.yaml.revrem.toml.secrets.baselineAGENTS.mdCHANGELOG.mdCODE_OF_CONDUCT.mdCONTRIBUTING.mdREADME.mdSECURITY.mdSUPPORT.mddocops.config.yamldocs/00-governance/docops-constitution.mddocs/00-governance/metadata.schema.jsondocs/00-governance/templates/adr.template.mddocs/00-governance/templates/fdd.template.mddocs/00-governance/templates/prd.template.mddocs/05-planning/plan-001-greenfield-docops-bootstrap-exercise.mddocs/05-planning/plan-002-tui-run-monitor-execution-deferral.mddocs/05-planning/tasks/task-001-public-github-launch-readiness.mddocs/10-prd/prd-001-interactive-tui-and-profile-system-for-code-review-loop.mddocs/45-adr/adr-001-package-the-review-loop-as-a-python-cli-with-a-companion-skill.mddocs/55-testing/test-001-utility-verification-strategy.mddocs/58-logs/log-001-public-launch-audit.mddocs/70-devex/devex-001-using-code-review-loop.mdengineering-principles-v1.1.mdpyproject.tomlscripts/dev-checkscripts/dev-pythonscripts/install-devscripts/promote-stablesrc/code_review_loop/__init__.pysrc/code_review_loop/__main__.pysrc/code_review_loop/cli.pysrc/code_review_loop/harnesses.pysrc/code_review_loop/profiles.pysrc/code_review_loop/progress.pysrc/code_review_loop/run_history.pysrc/code_review_loop/tui.pysrc/code_review_loop/tui_state.pytests/test_cli.pytests/test_devex_doc.pytests/test_harnesses.pytests/test_packaging.pytests/test_profiles.pytests/test_progress.pytests/test_run_history.pytests/test_tui.pytests/test_tui_state.py
Comment on lines
+308
to
+309
| Use the live CLI behavior and tests as the source of truth when docs lag. `MEMINIT-SPEC-008` is the normative v3 output contract (superseding MEMINIT-SPEC-004). | ||
|
|
There was a problem hiding this comment.
Align the spec reference version with the stated normative contract.
Line 308 states the normative contract is SPEC-008 v3, but Line 342 links to a ...-v2.md file. Please update one side so readers don’t follow an outdated contract.
Also applies to: 342-342
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/skills/meminit-docops/SKILL.md around lines 308 - 309, The spec
reference mismatch: update the doc so the normative contract and the linked spec
match by either changing the text that references MEMINIT-SPEC-008 (v3) or
replacing the link at line 342 that currently points to the v2 file; ensure the
document consistently references MEMINIT-SPEC-008 v3 (replace any `...-v2.md`
link with the v3 equivalent) and update any stray reference to MEMINIT-SPEC-004
to MEMINIT-SPEC-008 where intended.
Comment on lines
+4
to
+7
| url: https://github.com/GitCmurf/code-review-loop/security/advisories/new | ||
| about: Report vulnerabilities privately through GitHub Security Advisories. | ||
| - name: Support policy | ||
| url: https://github.com/GitCmurf/code-review-loop/blob/main/SUPPORT.md |
There was a problem hiding this comment.
Fix cross-repo links in issue contact routing.
Line 4 and Line 7 point to GitCmurf/code-review-loop instead of this repository, so vulnerability/support intake can be misrouted.
Suggested fix
- url: https://github.com/GitCmurf/code-review-loop/security/advisories/new
+ url: https://github.com/GitCmurf/revrem/security/advisories/new
...
- url: https://github.com/GitCmurf/code-review-loop/blob/main/SUPPORT.md
+ url: https://github.com/GitCmurf/revrem/blob/main/SUPPORT.md📝 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.
Suggested change
| url: https://github.com/GitCmurf/code-review-loop/security/advisories/new | |
| about: Report vulnerabilities privately through GitHub Security Advisories. | |
| - name: Support policy | |
| url: https://github.com/GitCmurf/code-review-loop/blob/main/SUPPORT.md | |
| url: https://github.com/GitCmurf/revrem/security/advisories/new | |
| about: Report vulnerabilities privately through GitHub Security Advisories. | |
| - name: Support policy | |
| url: https://github.com/GitCmurf/revrem/blob/main/SUPPORT.md |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/ISSUE_TEMPLATE/config.yml around lines 4 - 7, The URLs in the "url"
fields for security advisories and support policy are pointing to the hardcoded
repository "GitCmurf/code-review-loop" and should be updated to point to the
current repository dynamically or use the appropriate repository name to avoid
misrouting reports and support requests. Modify these URLs in the config.yml so
they reference the current repo's paths correctly rather than hardcoded external
paths.
Comment on lines
+3
to
+6
| on: | ||
| pull_request: | ||
| push: | ||
| branches: [main] |
There was a problem hiding this comment.
Release workflow lacks an inline test gate — flag before the first public tag push.
The PR objectives note that the release workflow triggers on v*.*.* tag pushes without running tests first. While this CI workflow (ci.yml) covers branches and PRs correctly, a release tag pushed directly to main (bypassing a PR) would publish artifacts without passing the test matrix. Consider adding a needs: test dependency in the release workflow, or requiring that tags are only pushed after the CI workflow passes on main.
This is outside ci.yml itself, but the gap is created by the combination of this workflow and the release workflow's trigger design.
Would you like me to generate a diff for the release workflow that adds a needs: test dependency to the publish job?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 3 - 6, The release workflow can be
triggered by v*.*.* tag pushes without running CI tests first; update the
release workflow's publish job (the job named "publish" or equivalent) to
require the test matrix by adding needs: test (or reference the test job name
used in the release workflow) so the publish job depends on the test job
completing successfully, or alternatively enforce in the release workflow that
tags are only allowed after successful CI on main; modify the release workflow
to include needs: test for the publish job to ensure tests run before
publishing.
Release workflow publishes without a test gate — address before the first tag push.
The PR objectives confirm the release workflow triggers on v*.*.* tag pushes without an inline test dependency. This CI workflow (ci.yml) correctly gates branches and PRs, but a tag pushed directly (bypassing a PR, or on a post-merge commit) skips the matrix entirely. Add needs: test (or equivalent) to the release workflow's publish job to enforce the test gate before any artifact reaches PyPI/GHCR.
Would you like me to generate a diff for the release workflow that adds a needs: test dependency to the publish job?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 3 - 6, The release workflow's publish
job is missing a dependency on the test job so tag pushes can bypass CI; update
the publish job (job name "publish") to include needs: test (or the actual test
job id, e.g., "test") so the publish step waits for the matrix/test job to
succeed before publishing on v*.*.* tag triggers; ensure the publish job's needs
array references the existing test job identifier used elsewhere in the
workflow.
| run: | | ||
| python -m pip install --upgrade pip | ||
| python -m pip install -e ".[dev]" | ||
| python -m pip install "meminit @ git+https://github.com/GitCmurf/meminit.git@63547bc79f46200d25e4b7375b5c661f64aa34f8" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does pip support integrity verification (hash checking) for git+ VCS dependencies the same way it does for PyPI packages with --require-hashes?
💡 Result:
Yes—partially. 1) PyPI / archive packages: --require-hashes (“hash-checking mode”) verifies local hashes provided in the requirements file via --hash per requirement, and it is all-or-nothing once enabled [1]. 2) VCS (git+) dependencies: pip does not treat VCS URLs like PyPI distributions with a single download artifact hash the same way. - Historically/behaviorally, pip can’t hash arbitrary VCS checkouts for --require-hashes mode and will error with messages like “Can’t verify hashes … because we don't have a way to hash version control repositories” [2]. - There is ongoing/added support to validate VCS URLs in hash-checking mode when the VCS reference is an immutable commit (e.g., a git commit hash). The work is described as “Validate VCS urls in hash-checking mode using their commit hashes,” aiming to “support immutable VCS references in hash-checking mode” [3]. The earlier issue that motivated this explicitly discusses rejecting VCS URLs unless they point to immutable commit hashes when --require-hashes is on [2]. So, the practical answer is: - If your git+ VCS requirement is pinned to an immutable git commit hash, pip can verify it in hash-checking mode using commit-hash validation (not the same mechanism as archive file --hashes) [3]. - If it is not pinned to an immutable revision (e.g., branch names/tags that can move, or other non-immutable refs), pip cannot provide equivalent hash-checking and will reject it under --require-hashes [2][3]. Recommendation: for repeatable/security-focused installs with git+ dependencies, pin to a full immutable git commit in the VCS URL; then use --require-hashes for the rest of your requirements [4][3].
Citations:
- 1: https://pip.pypa.io/en/stable/topics/secure-installs.html
- 2: Validate VCS urls in hash-checking mode using their commit hashes pypa/pip#6469
- 3: Validate VCS urls in hash-checking mode using their commit hashes pypa/pip#11968
- 4: https://pip.pypa.io/en/stable/topics/vcs-support/
Consider adding --require-hashes for stricter verification of the meminit git dependency.
The commit hash (63547bc79f46200d25e4b7375b5c661f64aa34f8) makes the dependency immutable, which is reproducible. However, pip only verifies git commit hashes in hash-checking mode (when --require-hashes is used); without it, pip will simply fetch whatever exists at that commit. If the commit is garbage-collected or the repository is deleted, CI will fail loudly.
For a dependency pinned to a git commit, consider:
- Adding
--require-hashesto enable pip's hash verification for the commit, or - Publishing a PyPI release so the dependency can be tracked through standard dependency management tools (Dependabot, etc.).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml at line 33, Update the CI pip install step that
installs the git-pinned meminit dependency by adding pip's hash-checking mode:
include the --require-hashes flag on the python -m pip install "meminit @
git+https://github.com/GitCmurf/meminit.git@63547bc79f46200d25e4b7375b5c661f64aa34f8"
invocation so pip enforces hash verification for the pinned commit; ensure the
command and any generated requirements file include matching hashes so the
install succeeds under --require-hashes.
Comment on lines
+13
to
+57
| jobs: | ||
| build: | ||
| name: Build release artifacts | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | ||
| with: | ||
| persist-credentials: false | ||
|
|
||
| - name: Set up Python | ||
| uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 | ||
| with: | ||
| python-version: "3.12" | ||
| cache: pip | ||
|
|
||
| - name: Build distributions | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| python -m pip install build twine | ||
| python -m build --sdist --wheel | ||
| python -m twine check dist/* | ||
|
|
||
| - name: Generate SBOM | ||
| uses: anchore/sbom-action@f8bdd1d8ac5e901a77a92f111440fdb1b593736b # v0.20.6 | ||
| with: | ||
| path: . | ||
| format: cyclonedx-json | ||
| output-file: dist/code-review-loop-sbom.cdx.json | ||
|
|
||
| - name: Attest build provenance | ||
| uses: actions/attest-build-provenance@e8998f949152b193b063cb0ec769d69d929409be # v2.4.0 | ||
| with: | ||
| subject-path: "dist/*" | ||
|
|
||
| - name: Upload workflow artifact | ||
| uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 | ||
| with: | ||
| name: release-artifacts | ||
| path: dist/* | ||
|
|
||
| - name: Publish GitHub release | ||
| uses: softprops/action-gh-release@6cbd405e2c4e67a21c47fa9e383d020e4e28b836 # v2.3.3 | ||
| with: | ||
| files: dist/* |
There was a problem hiding this comment.
Add a release-time verification gate before publishing artifacts.
This workflow publishes on tag push without running tests/checks first, so a bad tag can still produce a public release.
Suggested change
jobs:
+ verify:
+ name: Verify release candidate
+ runs-on: ubuntu-latest
+ steps:
+ - name: Checkout
+ uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
+ with:
+ persist-credentials: false
+ - name: Set up Python
+ uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0
+ with:
+ python-version: "3.12"
+ cache: pip
+ - name: Run quality gates
+ run: |
+ python -m pip install --upgrade pip
+ python -m pip install -e ".[dev]"
+ ./scripts/dev-check
+
build:
name: Build release artifacts
+ needs: verify
runs-on: ubuntu-latest📝 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.
Suggested change
| jobs: | |
| build: | |
| name: Build release artifacts | |
| runs-on: ubuntu-latest | |
| steps: | |
| - name: Checkout | |
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | |
| with: | |
| persist-credentials: false | |
| - name: Set up Python | |
| uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 | |
| with: | |
| python-version: "3.12" | |
| cache: pip | |
| - name: Build distributions | |
| run: | | |
| python -m pip install --upgrade pip | |
| python -m pip install build twine | |
| python -m build --sdist --wheel | |
| python -m twine check dist/* | |
| - name: Generate SBOM | |
| uses: anchore/sbom-action@f8bdd1d8ac5e901a77a92f111440fdb1b593736b # v0.20.6 | |
| with: | |
| path: . | |
| format: cyclonedx-json | |
| output-file: dist/code-review-loop-sbom.cdx.json | |
| - name: Attest build provenance | |
| uses: actions/attest-build-provenance@e8998f949152b193b063cb0ec769d69d929409be # v2.4.0 | |
| with: | |
| subject-path: "dist/*" | |
| - name: Upload workflow artifact | |
| uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 | |
| with: | |
| name: release-artifacts | |
| path: dist/* | |
| - name: Publish GitHub release | |
| uses: softprops/action-gh-release@6cbd405e2c4e67a21c47fa9e383d020e4e28b836 # v2.3.3 | |
| with: | |
| files: dist/* | |
| jobs: | |
| verify: | |
| name: Verify release candidate | |
| runs-on: ubuntu-latest | |
| steps: | |
| - name: Checkout | |
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | |
| with: | |
| persist-credentials: false | |
| - name: Set up Python | |
| uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 | |
| with: | |
| python-version: "3.12" | |
| cache: pip | |
| - name: Run quality gates | |
| run: | | |
| python -m pip install --upgrade pip | |
| python -m pip install -e ".[dev]" | |
| ./scripts/dev-check | |
| build: | |
| name: Build release artifacts | |
| needs: verify | |
| runs-on: ubuntu-latest | |
| steps: | |
| - name: Checkout | |
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | |
| with: | |
| persist-credentials: false | |
| - name: Set up Python | |
| uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 | |
| with: | |
| python-version: "3.12" | |
| cache: pip | |
| - name: Build distributions | |
| run: | | |
| python -m pip install --upgrade pip | |
| python -m pip install build twine | |
| python -m build --sdist --wheel | |
| python -m twine check dist/* | |
| - name: Generate SBOM | |
| uses: anchore/sbom-action@f8bdd1d8ac5e901a77a92f111440fdb1b593736b # v0.20.6 | |
| with: | |
| path: . | |
| format: cyclonedx-json | |
| output-file: dist/code-review-loop-sbom.cdx.json | |
| - name: Attest build provenance | |
| uses: actions/attest-build-provenance@e8998f949152b193b063cb0ec769d69d929409be # v2.4.0 | |
| with: | |
| subject-path: "dist/*" | |
| - name: Upload workflow artifact | |
| uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 | |
| with: | |
| name: release-artifacts | |
| path: dist/* | |
| - name: Publish GitHub release | |
| uses: softprops/action-gh-release@6cbd405e2c4e67a21c47fa9e383d020e4e28b836 # v2.3.3 | |
| with: | |
| files: dist/* |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 13 - 57, Add a release-time
verification gate by inserting a blocking verification step before the "Publish
GitHub release" step in the build job: run the project's full verification suite
(e.g., a step named "Verify artifacts" that runs linting, unit/integration tests
like python -m pytest and any packaging checks) so the job fails on bad
artifacts, and/or require a protected environment approval by adding environment
protection to the publish step (referencing the existing "Publish GitHub
release" step) so publishing is blocked until manual approvers approve; ensure
the new step runs after "Upload workflow artifact" and before "Publish GitHub
release" and aborts the workflow on non-zero exit so bad tags cannot publish.
Comment on lines
+53
to
+55
| if [ "$stable_venv_needs_rebuild" -eq 1 ]; then | ||
| rm -rf "$STABLE_VENV" | ||
| "$PYTHON" -m venv "$STABLE_VENV" |
There was a problem hiding this comment.
Guard destructive venv cleanup against unsafe paths.
rm -rf "$STABLE_VENV" runs on an env-overridable path; add a hard stop for empty or root-equivalent values before deleting.
Suggested change
if [ "$stable_venv_needs_rebuild" -eq 1 ]; then
+ case "$STABLE_VENV" in
+ ""|"/"|".")
+ printf '%s\n' "ERROR: refusing to remove unsafe STABLE_VENV path: '$STABLE_VENV'" >&2
+ exit 1
+ ;;
+ esac
rm -rf "$STABLE_VENV"
"$PYTHON" -m venv "$STABLE_VENV"
fi📝 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.
Suggested change
| if [ "$stable_venv_needs_rebuild" -eq 1 ]; then | |
| rm -rf "$STABLE_VENV" | |
| "$PYTHON" -m venv "$STABLE_VENV" | |
| if [ "$stable_venv_needs_rebuild" -eq 1 ]; then | |
| case "$STABLE_VENV" in | |
| ""|"/"|".") | |
| printf '%s\n' "ERROR: refusing to remove unsafe STABLE_VENV path: '$STABLE_VENV'" >&2 | |
| exit 1 | |
| ;; | |
| esac | |
| rm -rf "$STABLE_VENV" | |
| "$PYTHON" -m venv "$STABLE_VENV" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/promote-stable` around lines 53 - 55, The script currently does an
unconditional rm -rf "$STABLE_VENV" which can delete dangerous locations if
STABLE_VENV is empty or set to root-equivalent values; before the remove in the
block that checks stable_venv_needs_rebuild, add a guard that verifies
STABLE_VENV is non-empty and not one of unsafe values (e.g. "/" or "." or empty)
and optionally ensure it matches an expected pattern (for example starts with
the repo path or contains "venv" substring); if the check fails, print an error
and exit non‑zero instead of running rm -rf; reference the STABLE_VENV,
stable_venv_needs_rebuild, and PYTHON variables and perform the guard
immediately before rm -rf "$STABLE_VENV".
|
|
||
| Use GitHub Private Vulnerability Reporting as the primary channel: | ||
|
|
||
| `https://github.com/GitCmurf/code-review-loop/security/advisories/new` |
There was a problem hiding this comment.
Fix the private advisory URL to the current repository.
Line 13 points to GitCmurf/code-review-loop, which appears mismatched with this repository and can break private vulnerability intake.
Proposed fix
-`https://github.com/GitCmurf/code-review-loop/security/advisories/new`
+`https://github.com/GitCmurf/revrem/security/advisories/new`📝 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.
Suggested change
| `https://github.com/GitCmurf/code-review-loop/security/advisories/new` | |
| `https://github.com/GitCmurf/revrem/security/advisories/new` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SECURITY.md` at line 13, The SECURITY.md file contains a hard-coded private
advisory URL
`https://github.com/GitCmurf/code-review-loop/security/advisories/new` that
points to the wrong repository; update that URL to use this repository's correct
GitHub path (replace `GitCmurf/code-review-loop` with the current repo
owner/name) so private vulnerability intake opens on the right repo and verify
the revised link format matches GitHub's advisory URL pattern.
Comment on lines
+160
to
+163
| def _codex_config_args(reasoning_effort: str | None) -> list[str]: | ||
| if reasoning_effort is None: | ||
| return [] | ||
| return ["-c", f'model_reasoning_effort="{reasoning_effort}"'] |
There was a problem hiding this comment.
reasoning_effort is embedded in the -c config argument without validation.
The format string f'model_reasoning_effort="{reasoning_effort}"' directly inserts the user-supplied value. If a profiles.toml entry contains a double-quote in the value (e.g. reasoning_effort = 'hig"h'), the resulting argument is model_reasoning_effort="hig"h", which is malformed for the codex CLI's -c key=value parser. Add a guard that restricts the value to safe characters (e.g. alphanumeric/hyphens):
🛡️ Proposed fix
def _codex_config_args(reasoning_effort: str | None) -> list[str]:
if reasoning_effort is None:
return []
+ if not reasoning_effort.replace("-", "").replace("_", "").isalnum():
+ raise ValueError(
+ f"reasoning_effort must contain only alphanumeric, hyphen, or underscore characters; got {reasoning_effort!r}"
+ )
return ["-c", f'model_reasoning_effort="{reasoning_effort}"']🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/code_review_loop/harnesses.py` around lines 160 - 163, The
_codex_config_args function currently interpolates reasoning_effort directly
into the '-c' argument, which breaks if the value contains quotes or other
unsafe chars; validate that reasoning_effort matches a restricted pattern (e.g.
only alphanumerics and hyphens) before building the argument: use a regex
full-match (e.g. r'^[A-Za-z0-9-]+$') to accept the value, and if it fails
validation either return an empty list or raise a ValueError (choose consistent
behavior with the module) instead of interpolating the raw string; update
_codex_config_args to perform this check and only produce ["-c",
f'model_reasoning_effort="{reasoning_effort}"'] when the validation passes.
Comment on lines
+17
to
+29
| version_line = next(line for line in front_matter if line.startswith("version: ")) | ||
|
|
||
| history_start = lines.index("## Version History") | ||
| history_rows = [ | ||
| line | ||
| for line in lines[history_start + 1 :] | ||
| if line.startswith("| ") and not line.startswith("| Version ") and not line.startswith("|---") | ||
| ] | ||
|
|
||
| latest_row = history_rows[0] | ||
| latest_version = latest_row.split("|")[1].strip() | ||
|
|
||
| assert version_line == f"version: '{latest_version}'" |
There was a problem hiding this comment.
Harden this doc-version test to avoid brittle false failures.
Line 26 can throw an unhelpful IndexError when no history rows are found, and Line 29 depends on a specific quote style (version: 'x.y') rather than semantic version equality.
Proposed patch
def test_devex_front_matter_version_matches_latest_history_row():
lines = DOC_PATH.read_text(encoding="utf-8").splitlines()
@@
version_line = next(line for line in front_matter if line.startswith("version: "))
+ front_matter_version = version_line.split(":", 1)[1].strip().strip("'\"")
@@
history_rows = [
line
for line in lines[history_start + 1 :]
if line.startswith("| ") and not line.startswith("| Version ") and not line.startswith("|---")
]
+ assert history_rows, "Version History table must contain at least one data row"
@@
latest_row = history_rows[0]
latest_version = latest_row.split("|")[1].strip()
- assert version_line == f"version: '{latest_version}'"
+ assert front_matter_version == latest_version🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_devex_doc.py` around lines 17 - 29, The test currently assumes
history_rows has at least one entry and that the front_matter version string
uses single quotes, which can raise IndexError or false negatives; update the
test to first assert history_rows is non-empty with a clear failure message (so
the test fails meaningfully if the Version History table is missing), compute
latest_row = history_rows[0] only after that guard, extract latest_version from
latest_row and normalize both the extracted latest_version and the version_line
value to semantic versions (e.g., strip surrounding quotes and whitespace)
before comparing, and assert equality of the normalized versions rather than
string equality (use the variables version_line, history_rows, latest_row,
latest_version, and front_matter to locate the code to change).
Comment on lines
+8
to
+10
| def test_append_and_read_history_round_trip(tmp_path, monkeypatch): | ||
| monkeypatch.setenv("HOME", str(tmp_path / "home")) | ||
| summary = { |
There was a problem hiding this comment.
Make this test deterministic by clearing XDG_DATA_HOME.
Line 9 sets HOME, but if XDG_DATA_HOME is present in the runner environment, Line 30’s expected path can fail nondeterministically. Clear XDG_DATA_HOME in this test setup.
Suggested patch
def test_append_and_read_history_round_trip(tmp_path, monkeypatch):
+ monkeypatch.delenv("XDG_DATA_HOME", raising=False)
monkeypatch.setenv("HOME", str(tmp_path / "home"))Also applies to: 30-30
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_run_history.py` around lines 8 - 10, In
test_append_and_read_history_round_trip, clear the XDG_DATA_HOME environment
variable before configuring HOME so the expected path is deterministic: call
monkeypatch.delenv("XDG_DATA_HOME", raising=False) (or
monkeypatch.setenv("XDG_DATA_HOME", "") if you prefer) at the top of the test
(before monkeypatch.setenv("HOME", ...)) to ensure the test does not pick up the
runner's XDG_DATA_HOME.
12 tasks
Merged
GitCmurf
added a commit
that referenced
this pull request
Jun 23, 2026
* docs: next phase planning docs * docs: iteration r1 * docs: CC revision, adding CI env-var awareness * docs: CC revision r3 * docs: CC revision r4 * docs: CC revision r5 * docs: CC revision r6 * docs: CC revision r7 * feat(t0): v0.5.0 fixture catalogue + PLAN-005 governance (REVREM-PLAN-005 T0) Stand up the shared inputs every v0.5.0 read-only consumer (revrem report, the GitHub Action) reads from a finished run directory. - Register PLAN-005 in the governed planning tree: move docs/next-steps-v0.5.0.md -> docs/05-planning/plan-005-next-steps-v0.5.0.md (meminit check green; document_id REVREM-PLAN-005 preserved). - Assemble a finished-run fixture catalogue under tests/fixtures/runs/<scenario>/ co-locating summary.json + events.jsonl for 8 terminal-state scenarios: clear, findings_remediated, findings_remaining, timeout, check_failure, cost_ceiling, cancelled, all_suppressed. These did not exist as combined run dirs before (summary/events fixtures were split across two trees with mismatched names); the four scenarios that had no summary are authored to summary-v1 shape with final_status/stopped_reason matching their events. - Add tests/support/run_fixtures.py::load_run(name) so downstream tests address a scenario by stable name. - Add tests/test_run_fixtures.py: meta-test asserting every scenario has both files and that summary validates against summary-v1 and events against events-v1 (25 cases, reusing the _compat_jsonschema validator). - Update REVREM-TEST-001: reference the catalogue and record the v0.5.0 report + Action success metrics. Gate: pytest (1300+ pass; 1 pre-existing env failure unrelated to T0 — test_cli_config_commands shells out to bare 'python' absent in WSL), ruff clean, mypy clean, lint-imports green (9 contracts), meminit check green (only a pre-existing FRONTMATTER_MISSING on an unrelated handoff doc), git diff --check clean. All fixture files verified LF-clean (no CRLF) for golden determinism. * feat(t1): revrem report subcommand — static HTML + JSON index (REVREM-PLAN-005 T1) Add a read-only subcommand that renders a finished run's summary.json + events.jsonl into a single self-contained HTML file, or a machine-readable JSON index. Never invokes a model or touches the network (gate G5). - src/code_review_loop/report_html.py: pure render_report(summary, events, *, redact=True) -> str and build_report_index(...) -> dict. Plain HTML5 + inline <style>, no JS/external assets, html.escape on all interpolation. Redaction on by default for anything leaving the run dir (gate G7). Deterministic: timestamps read from inputs (never now()), filesystem paths normalized via PurePosixPath so os.sep never leaks (Contract #9). - src/code_review_loop/cli/commands/report.py: command handler mirroring replay.py + bundle.py redaction-pair enforcement (--no-redact requires --i-understand-the-risks, exit 4). HTML writes via write_text_artifact (atomic); --format json prints canonical JSON to stdout (--output ignored). Truncated/malformed events render what is available + warn + exit 0 (the report is diagnostic). - cli/args.py: parse_report_args (run_dir, --output/-o, --format {html,json}, --no-redact, --i-understand-the-risks). registry.py: register 'report'. - docs/52-api/schemas/report-index-v1.schema.json + _history baseline: the frozen cross-boundary contract (Contract #8) describing the index's fields and nullability (cost_usd null for fake harness; top_findings [] when none; artifact_paths {} when unavailable). Auto-covered by the schema-parity test. - Tests: test_report_html.py (render over all 8 fixtures, determinism, no-CRLF, self-containment, redaction default, CLI exit codes, truncated tolerance, index required-keys + nullability); test_report_json.py (every index validates vs schema, --format json to stdout, --output ignored, golden snapshot of the clear-run index). test_cli_dispatch.py updated for the new subcommand. - Docs: DEVEX-001 'Static HTML report' subsection + version history; README Key Features bullet. Gate: 1371 passed (1 pre-existing env failure in test_cli_config_commands unrelated to T1); ruff, mypy, lint-imports (9 contracts), meminit check (only a pre-existing unrelated FRONTMATTER_MISSING), git diff --check all clean. * feat(t2): report body sections + golden HTML + determinism guards (REVREM-PLAN-005 T2) Fill the report body with the substance a reviewer wants and lock it with byte-stable golden files. - report_html.py: add four body sections rendered honestly from summary/events data (or 'unavailable' when absent): * Findings & triage: configured findings (severity, path, summary, f1: fingerprint) from status_classification events; suppressed findings with provenance; pointer to triage-N.json when present. * Checks: command/status/message/iteration from check_result events. * Cost & budget: tokens, USD, charge events, ceiling breach. Unreported costs render 'not reported', never a fabricated 0 (renders null honestly). * Diff stats: only from in-artifact data; absent -> 'diff stats unavailable for this run'. The report NEVER shells out to git. Also render tokens dict as key=value pairs instead of a raw repr. - tests/golden/report/: committed golden HTML for 6 fixtures (clear, findings_remediated, findings_remaining, check_failure, cost_ceiling, all_suppressed), byte-stable across runs and platforms. - test_report_html.py: golden comparison with REVREM_UPDATE_SNAPSHOTS=1 regeneration affordance; cross-platform guards asserting the committed goldens contain no CRLF and no backslash path separators (durable on the golden, not just the live render); content assertions for each terminal state (remediated shows the passing check + clear; all_suppressed shows the stop reason; cost_ceiling shows the breach + error badge; null cost renders 'not reported' not 0; diff stats 'unavailable' path). - DEVEX-001: expand the report subsection with a description of each body section in order, plus the determinism/stability note (HTML layout not a stable contract until 0.9.0; only report-index-v1 fields are versioned). Gate: 1388 passed (1 pre-existing env failure unrelated to T2); ruff, mypy, lint-imports (9 contracts), git diff --check all clean. * feat(t3): headless/CI output hardening — --no-tty + CI=true (REVREM-PLAN-005 T3) A CI run must produce ANSI-free stderr with no flags beyond what the provider sets automatically. Add a single suppression gate, progress.force_terminal(), that fires on either --no-tty or the CI env var (set automatically by GitHub Actions and most other providers). - progress.py: force_terminal(no_tty) helper returning isatty and not CI and not no_tty. Thread no_tty into both Console sites (_console_and_text, rich_live_progress); rich_live_progress now accepts an optional no_tty kwarg (backward compatible). - adapters/terminal.py: terminal_title_supported now returns False when config.no_tty or CI is set. This guards the /dev/tty write path, which a stderr-capture test would miss because /dev/tty exists on many CI runners regardless of isatty(). - Threading (additive throughout): OutputConfig.no_tty (routing_types) + OUTPUT_KEYS 'no_tty' (profiles) + LoopConfig.no_tty (config) + config_builder pick(args.no_tty, profile.output.no_tty, False) + args.py --no-tty flag (store_true) + runner.py passes config.no_tty into rich_live_progress. Also exposed as an additive profile key [output] no_tty. - tests/test_headless_output.py (19 cases): force_terminal unit tests (--no-tty, any CI value, isatty baseline); terminal_title_supported gate tests; rich_live_progress ANSI-free-under-no_tty; end-to-end test driving a real loop (rich progress + no_tty) asserting zero CSI bytes on captured stderr; documented exit-code mapping stability (0/2/3/4/5). - DEVEX-001: headless/CI guidance + recommended invocation (--progress-style compact --no-tty --summary-format json) + always-write- before-exit note; version history. Gate: 1407 passed (1 pre-existing env failure unrelated to T3); ruff, mypy, lint-imports (9 contracts), devex-doc version check all clean. * feat(t4): reference GitHub Action + idempotent PR comment (REVREM-PLAN-005 T4) A composite Action that runs a revrem profile on a PR, uploads a redacted HTML report, and posts a single updatable PR comment. Depends on T2 (report) + T3 (headless stability). - action.yml: composite action. Inputs: base, profile, max-iterations, max-wall-seconds, max-usd, max-tokens, checks (newline list), comment, upload-artifacts, raw-artifacts, fail-on-findings, install-mode (pypi|local), revrem-version. Does NOT checkout (caller must, fetch-depth:0); verifies base ref (exit 4 if absent). Runs revrem headless (--no-tty --progress-style compact --summary-format json); splits 'checks' via a bash while-read into repeated --check flags (never interpolates the joined string = injection vector). Two-stage guard: parse JSON stdout first, then read artifact_dir (never glob .revrem/runs/); fast-fail exit 4 if revrem crashed pre-summary. Always render/upload/comment when RUN_DIR discovered regardless of exit; maps exit code LAST (0 pass; 2 fail-on-findings gate; 3 budget; 4/5 error). Fork-PR safe: comment step gated on head.repo.fork != 'true' (never pull_request_target). Comment script invoked via ${{ github.action_path }} prefix for external users. - scripts/ci/post_pr_comment.py (stdlib only): pure build_comment_body(index) separated from the HTTP layer. Reads the already-redacted revrem-report.json via json.load; marker <!-- revrem-report --> update-or-create; body bounded (top 5 findings, fixed summary rows); never embeds raw model output/stderr; degraded generic comment when report unreadable. - .github/workflows/revrem-pr.yml: dogfood workflow (local install, dogfood profile), least-privilege (contents:read, pull-requests:write). - tests/support/fake_github_api.py: http.server stub for GET comments + POST/ PATCH comment (records requests, no network). - tests/test_post_pr_comment.py (13 cases): pure body builder (marker, status, findings, cost n/a when null, run url, never-embeds-secrets-by-design); posting vs stub (create-then-update idempotency, marker is the key, unrelated comment untouched, comment-before-fail ordering, main reads report + posts, degraded on unreadable, requires credentials). - tests/test_action_yaml.py (10 cases): action.yml + workflow parse; structural contract (composite runs, required inputs, pypi/local modes, headless json invocation, artifact_dir discovery not globbing, checks splitting, fork gate, exit mapping is last step, action_path prefix, dogfood local install). - README + DEVEX-001: Hands-off CI section (usage, least-privilege, fork model, redacted upload, generic-provider path); version history. Gate: 1430 passed (1 pre-existing env failure unrelated to T4); ruff, mypy, lint-imports (9 contracts), devex-doc version check all clean. action.yml + revrem-pr.yml parse as valid YAML (10 structural tests). * release(t13): cut v0.5.0 (REVREM-PLAN-005 T13) The 'showcase & hands-off adoption' release: Tier 1 (Pillars A+B) of REVREM-PLAN-005 — static HTML report, headless/CI hardening, and the reference GitHub Action. Pillars C (expert profiles) and D (DevEx) deferred to v0.5.x; T12 (TUI real runs) deferred to v0.6.0 per the plan's decision gate. - Version 0.4.0 -> 0.5.0 in pyproject.toml AND src/code_review_loop/__init__.py (release.yml validates the tag against __init__.__version__). - action.yml default revrem-version -> 0.5.0. - CHANGELOG [0.5.0]: Added (report, report-index-v1 schema, GitHub Action, --no-tty + CI detection, run fixture catalogue); Changed (recommended CI invocation); Stability (frozen schemas + summary/events; Preview report HTML layout + expert-profile surface); Deferred (Pillar C/D to v0.5.x, T12 to v0.6.0). - Fix: the golden-HTML test compared against tests/golden/report/ but the path resolved to a stray top-level golden/, so the comparison was a no-op (always re-wrote and passed). Correct _GOLDEN_DIR to tests/golden/report/; regenerate the committed goldens so they now genuinely pin the render (and embed 0.5.0). Remaining (manual, before tagging): dry-run release.yml; >=1 live dogfood run (revrem --profile dogfood) with revrem report rendering it; live observation that the Action posts a single idempotent PR comment within ~5 min on a real PR. These are recorded in the CHANGELOG and cannot be automated here. Gate: 1430+ passed (1 pre-existing env failure in test_cli_config_commands); ruff, mypy, lint-imports (9 contracts), meminit check (only a pre-existing unrelated FRONTMATTER_MISSING), git diff --check all clean. * fix(c1): render findings from triage-N.json::confirmed_findings (P0-1) The headline defect from peer review: the renderer derived findings from status_classification events whose payload was assumed to be severity-keyed lists. That shape does not exist in the engine — real status_classification payload is {message, summary} (verified across .revrem/runs/). Real confirmed findings live in triage-N.json::confirmed_findings. Result before this fix: findings section, finding_counts, and top_findings were empty on every real run, and the PR comment reported 0 findings. Tests were green only because the T0 fixtures were hand-authored to match the renderer's assumptions, not the engine's contract (self-confirming goldens). - report_html.py: add triage_findings param to render_report and build_report_index. Source confirmed/suppressed findings from parsed triage payloads (_flatten_confirmed_findings / _flatten_suppressed_findings). finding_counts and top_findings now derive from triage confirmed_findings (severity, affected_paths, summary, fingerprint, rationale), not the status_classification event. The renderer stays pure — it receives parsed triage, never reads disk. - cli/commands/report.py: _load_triage_findings loads triage-N.json from summary.artifact_paths.triage (relative to run dir; highest N authoritative), threads parsed findings into both render paths. Missing/unreadable triage is skipped gracefully. - Red gate: tests/test_report_real_findings.py + a real-shaped findings_with_triage fixture (status_classification payload {message,summary} — no severities; triage-1.json with one confirmed medium finding). Was red before this commit for exactly the right reason; now green. Registered as a standing gate in REVREM-TEST-001 (C5). - Fixtures made representative: all_suppressed + findings_remaining now carry triage-1.json with confirmed/suppressed findings (findings_remaining had a triage-less summary despite its name). findings_with_triage is a new fixture whose status_classification matches the real engine shape. - Regenerated goldens: findings_remaining.html now shows the finding (fingerprint + summary), not 'No configured findings recorded'. Verified against ground truth: real status_classification payloads across .revrem/runs/ carry only {message, summary}; real findings live in triage-N.json with keys confirmed_findings/rejected_findings/needs_more_info/ suppressed_findings, each finding {severity, summary, affected_paths, fingerprint, rationale}. Gate: 1433 passed (1 pre-existing env failure unrelated); ruff, mypy, lint-imports (9 contracts) clean. * fix(c2): remove dead phase_config section (P0-2) _phase_config_rows read summary.phase_config, but summary-v1 has no such key (0 matches in the schema; verified). The section returned '' on every run — dead code that faithfully implemented a plan referencing a nonexistent field. The header already shows the real top-level summary.harness. - report_html.py: delete _phase_config_rows and its call in render_report. - test_report_html.py: assert the header renders summary.harness and that no 'Phase configuration' section appears. - Regenerated goldens (section removed). Gate: ruff, mypy clean; report tests green. * fix(c3): wire github-token input for the PR comment (P0-3) The comment step read GITHUB_TOKEN: ${{ env.GITHUB_TOKEN }}, but GITHUB_TOKEN is not a default env var for composite actions and the dogfood workflow never set it. So env.GITHUB_TOKEN resolved empty -> post_pr_comment.py hit its 'required' guard and exited 1; no comment was ever posted. The test suite missed this because test_post_pr_comment.py called post_or_update_comment directly, bypassing action.yml's wiring. - action.yml: add a github-token input defaulting to ${{ github.token }} (composite actions can't read secrets, so the caller passes it in). The comment step now reads GITHUB_TOKEN: ${{ inputs.github-token }}. The dogfood workflow needs no extra secret wiring (defaults to the automatic token). - test_action_yaml.py: assert the github-token input exists and defaults to github.token; assert the comment step's GITHUB_TOKEN comes from inputs.github-token, NOT env.GITHUB_TOKEN (the original bug). Live verification (a comment actually posts on a PR) is a manual gate recorded in C7 and the CHANGELOG. * fix(c4): render checks from summary.iterations[].checks[] (engine real source) The checks section previously read only check_result events. The engine's authoritative check record is summary.iterations[].checks[] ({command, status, artifact}) — verified across real .revrem/runs/. Now source from the summary, falling back to check_result events for older runs that carry none. - report_html.py: _checks_section takes summary, iterates iterations[].checks[] (command/status/artifact link/iteration), falling back to check_result events. Column renamed Message->Artifact (the real field). - findings_with_triage fixture: add a real-shaped summary check (ruff check ., passed, check-1-1.txt artifact) so C4 is exercised. - test_report_real_findings.py: assert the summary-sourced check renders and the empty placeholder does not appear. - Regenerated goldens. Gate: ruff, mypy, lint-imports clean; 116 report/fixture tests green. * docs(c5): register real-findings render gate in REVREM-TEST-001 Add a standing success-metric row for tests/test_report_real_findings.py — the anti-self-confirming-fixture gate introduced by the C1 corrective. It asserts findings surface from triage-N.json::confirmed_findings (and checks from summary.iterations[].checks[]) on a real-engine-shaped fixture, and fails if that ever stops happening. Document why the gate exists (T0 fixtures were hand-authored against the renderer's assumptions, not the engine's contract). Fixture representativeness for the P0-relevant scenarios (findings_remaining, all_suppressed, findings_with_triage) was restored in C1/C4 — they now carry real triage and checks. Full regeneration of every fixture from .revrem/runs/ (including the 5 secondary schema keys the review noted: external_review_coverage, invocation, phase_failures, phase_observations, triage_diagnostics) is tracked as a follow-up; it does not gate the showcase features. REVREM-TEST-001 bumped 1.7 -> 1.8. * fix(c6): deep-link the report artifact in the PR comment (secondary) The comment always linked the workflow run URL, never the uploaded report artifact. The plan specified capturing actions/upload-artifact's artifact-url output with the run URL as fallback. - action.yml: give the upload-report step an id; add an Export-artifact-URL step that writes the artifact-url output to REVREM_ARTIFACT_URL env; pass it into the comment step env. - post_pr_comment.py: main() prefers REVREM_ARTIFACT_URL over GITHUB_RUN_URL. - test: assert the posted body links the artifact URL and not the run URL. Gate: ruff clean; 25 comment/action tests green. * docs(c7): document corrective fixes + release hygiene (P0 series) No v0.5.0 tag was ever created (git tag -l 'v0.5*' is empty) — the release(t13) commit bumped the version in source but never tagged. So there is no premature tag to revert; 0.5.0 remains unreleased as intended. CHANGELOG [0.5.0] gains a Fixed (corrective) section documenting P0-1/P0-2/P0-3 plus the checks-source, artifact-deep-link, and real-findings-gate fixes. Remaining manual gates before tagging v0.5.0 (cannot be automated here, recorded in the CHANGELOG): - A live dogfood revrem --profile dogfood run with revrem report rendering it. - A live observation that the Action posts a single idempotent PR comment on a real PR (validates the C3 github-token wiring end-to-end). Gate: 1437 passed (1 pre-existing env failure unrelated); ruff, mypy, lint-imports (9 contracts) clean. * fix(c8): document phase_config in summary-v1 schema (P0-B root cause) phase_config is written on every run by reporting.add_summary_contract_fields (present on 106/156 local runs, all recent ones) but was undocumented in summary-v1.schema.json. additionalProperties was already true, so validation never caught the drift — which is exactly why the earlier P0-2 wrongly concluded the key did not exist and deleted the report's phase-config section. Additive, non-breaking (v1 is additive-only; 5 other optional keys already use this convention). _history byte-equality baseline updated to match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TMtj14Nxj7teRfW7xz6mtE * fix(c9): make findings_with_triage fixture representative (P0-B) The hand-authored fixtures omitted every key a real run carries, which is what let the self-confirming-fixture failure mode persist. Make the anchor fixture representative of a real run: - summary.json: real-shaped phase_config (review/triage/remediation/ commit_message with harness/model/effort; plus the non-model checks/runtime blocks) and the five previously-omitted keys (external_review_coverage, invocation, phase_failures, phase_observations, triage_diagnostics). - triage-1.json: a rejected_finding and a needs_more_info entry so the renderer is exercised against the full triage outcome, not confirmed-only. Inert until the renderer reads it (next commit); suite stays green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TMtj14Nxj7teRfW7xz6mtE * fix(c10): restore phase-config section + render full triage outcome (P0-A, C1) Reverts the wrong P0-2 deletion and completes the C1 findings contract. - Restore the "Phase configuration" section, sourced from summary.phase_config (real data on every modern run). Filter to the model-bearing phases (review/triage/remediation/commit_message); checks/runtime carry no harness/model and are skipped, not emitted as blank rows. Now also surfaces reasoning-effort, timeout, and sandbox. - Render triage rejected_findings and needs_more_info, which were silently dropped (only confirmed/suppressed rendered before). - Surface per-iteration iterations[].review_status in the outcome section (the _findings_section docstring already promised this; it is now true). Goldens regenerated: the sole delta is the inserted Review status row (verified by word-diff). New gate assertions in test_report_real_findings.py cover the phase-config table + filter, rejected/needs-info, and review_status. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TMtj14Nxj7teRfW7xz6mtE * fix(c11): fix the inert fork-PR guard in the Action (Sec-1) `github.event.pull_request.head.repo.fork != 'true'` always evaluated true under GitHub Actions' type coercion (boolean true -> 1, string 'true' -> NaN, 1 != NaN), so the comment step never actually skipped on fork PRs — where the token is read-only and the post would 403 and fail the job. Compare to the boolean `false` instead. Static fix; requires live verification on a real fork PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TMtj14Nxj7teRfW7xz6mtE * fix(c12): label the comment artifact link [Report], not [Run] (Sec-3) build_comment_body linked the uploaded report artifact (REVREM_ARTIFACT_URL) but labelled it [Run]. Pass report_url and run_url separately: deep-link the artifact as [Report] when captured, fall back to the workflow run as [Run] otherwise. Comment stays confirmed-findings-only by design (the actionable "what remains" summary); rejected/needs-info live in the full HTML report. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TMtj14Nxj7teRfW7xz6mtE * docs(c13): record second corrective iteration + extend the render gate - CHANGELOG: correct the stale P0-2 entry (it described the now-reverted deletion) and document the second corrective iteration (phase-config restore, schema drift, fork gate, rejected/needs-info, comment label, fixture). - REVREM-TEST-001: extend the real-findings gate entry to cover the new regression guards (phase-config table + filter, rejected/needs-info, review_status) and the fixture's representativeness. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TMtj14Nxj7teRfW7xz6mtE * fix(c14): preserve array-valued artifact paths + redact index paths (GPT #3, #7) build_report_index normalised every artifact_paths value with _normalize_path, flattening list entries (triage, reviews, ...) to repr strings like "['triage-1.json']" — even though report-index-v1 allows arrays and the PR comment / consumers rely on the real shape. Preserve lists as lists of normalised strings, and redact each path element when redact=True (an absolute local run dir under $HOME is a real leak vector; redact_text scrubs it to [REDACTED:home]). Also tighten the review-status gate to assert the rendered value, not just the label. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TMtj14Nxj7teRfW7xz6mtE * fix(c15): sanitize PR comment markdown table cells (GPT #6) A model-derived finding title containing a pipe, newline, or backtick could add columns, split the table row, or open an unterminated code span in the comment. Route severity/file/title through an _md_cell escaper (escape `|`, collapse line breaks, neutralise backticks) before interpolating into the table. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TMtj14Nxj7teRfW7xz6mtE * fix(c16): harden the Action — injection, token fallback, raw-artifact accuracy (GPT #1, #2, #4, #5) - #1 Script injection: every workflow input is now passed through `env:` and read as a shell variable ("$BASE", "$CHECKS", ...), never interpolated as `${{ inputs.x }}` into a run script (GitHub expands those into the script text before bash parses it). Added numeric validation for the budget inputs and a test asserting no `${{ inputs.* }}` appears in any run script. - #2 Token: the comment step reads `${{ inputs.github-token || github.token }}` so the token resolves even if expression evaluation inside the input default is not honoured. - #4 Raw upload: `raw-artifacts` is documented as uploading the run directory VERBATIM and UNREDACTED (raw prompts/model output/paths) — the prior "still-redacted" wording was false. DevEx doc updated to match. - #5 Doc drift: DevEx doc now documents the corrected `head.repo.fork == false` fork guard; added a test asserting doc/action agreement. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TMtj14Nxj7teRfW7xz6mtE * docs(c17): record external-review security & robustness fixes Document the GPT-review corrective iteration (script-injection hardening, token fallback, raw-artifact honesty, array-path preservation + redaction, comment cell escaping) in the Unreleased corrective section. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TMtj14Nxj7teRfW7xz6mtE * fix(c18): make github-token default empty so the token fallback works (GPT re-review) The c16 fallback `${{ inputs.github-token || github.token }}` was defeated by the input default `${{ github.token }}`: if GitHub does not evaluate the expression in the default, inputs.github-token becomes the literal non-empty string "${{ github.token }}" — which is truthy, so `||` short-circuits to it and the action sends a bad token. Exactly the failure mode the fallback was meant to prevent. Set the default to "" (empty, not an expression). When the caller omits the input, inputs.github-token is falsy and the fallback resolves github.token in the step context, where expressions are honoured. Test asserts the default is empty; CHANGELOG/docs corrected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TMtj14Nxj7teRfW7xz6mtE * fix: PR review comments (GLM) * fix: PR review comments 2 (GLM/CC) * fix: PR review comments 3 (CC) * fix: CI gate (CC) * fix: PR review comments 4 (CC) * fix: make action metadata load in GitHub * fix: keep action scratch files outside checkout * fix: bootstrap codex in dogfood action * fix: install dogfood check tools in action workflow * fix: make dogfood action codex-only in CI * docs: added tech debt note * fix: PR review comments 5 (CC) * fix: configure git author for dogfood commits * fix: bound dogfood action runtime * fix: resolve dogfood report findings * fix: sanitize stop reason in PR comment * fix: report latest triage artifact only * fix: emit summaries for typed CLI failures * fix: surface phase failures in dogfood reports * fix: include provider excerpts in failure diagnostics * fix: classify codex quota exhaustion * fix: surface quota exhaustion in ci * fix: use portable action diagnostics lookup [skip ci] * fix: address review feedback on report action safety [skip ci] * fix: normalize windows report paths [skip ci] * test: add no-provider action smoke workflow * fix: address report and dogfood review comments --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
document_id: REVREM-LOG-001
type: LOG
title: Public launch audit
status: Draft
version: '0.1'
last_updated: '2026-05-07'
owner: GitCmurf
docops_version: '2.0'
area: release
description: Audit record for preparing code-review-loop for public GitHub launch
keywords:
related_ids:
LOG: Public launch audit
Context
This log records the local audit evidence generated while executing
REVREM-TASK-001for the first public GitHub launch ofcode-review-loop.GitHub-side settings, initial launch PR creation, and bot review cannot be
completed until the public remote exists, but the local repository surface and
verification gates can be prepared in advance.
Content
Safety Point
2d93138pre-public-audit-2026-05-07The safety tag is local-only and must not be pushed to the public remote.
Local Content Added
LICENSEtext and projectNOTICE.README.mdrewritten around installation, safety model, artifacts,profiles, TUI, development, release, and licensing.
CONTRIBUTING.md,CODE_OF_CONDUCT.md,SECURITY.md, andSUPPORT.md..github/CODEOWNERS, pull request template, YAML issue forms, andDependabot configuration.
provenance/SBOM generation.
.env.example,.secrets.baseline,.editorconfig, expanded.gitignore,and stronger
.pre-commit-config.yaml.Secret And PII Scan Evidence
detect-secrets scanbaseline generation.secrets.baselinewith zero findings after excluding generated caches and virtualenv/build artifacts.git ls-files --cached --others --exclude-standard -z | xargs -0 ./.venv/bin/detect-secrets-hook --baseline .secrets.baseline.env.example, test decorators, and governed launch-plan text. No credential or private transcript found.command -v gitleaksgitleaksis not installed in this environment; use GitHub-side workflow or install before public launch for an additional scanner pass.History Scan Evidence
git log --all --name-onlytargeted scan.revrem.toml,run_history.py, andtest_run_history.py; no committed transcript,.env, private key, or obvious credential artifact path found.git rev-list --all ... git grepkeyword scangit fsck --no-reflogsDependency License Review
Runtime dependencies remain empty. Development and optional dependency review
using
pip-licensesfound permissive licenses for the direct launch-relevanttools:
builddetect-secretsmypypre-commitpytestrichrufftextualtwineNo third-party NOTICE obligation was identified for the current runtime
dependency set. Re-run the dependency review before the first public artifact
release because release tooling and optional extras may change.
Verification Evidence
./scripts/dev-check218 passed; ruff, mypy, Meminit doctor/check, andgit diff --checkpassed.pre-commit run --all-files --show-diff-on-failurememinit check --format jsongit diff --checkpython -m build --sdist --wheelcode_review_loop-0.3.0.tar.gzandcode_review_loop-0.3.0-py3-none-any.whl.python -m twine check dist/*uv.lockwas refreshed after adding launch-time development tools and afterupdating package license metadata to the SPDX expression form.
Deferred External Actions
These items require the public GitHub repository to exist:
main;security updates;
Note
Add packaging, CI, tests, and public launch documentation for code-review-loop
code-review-loop/revremconsole scripts, packaging metadata, and optionalprogress/tuiextras; version is0.3.0requiring Python >=3.11harnesses.py(codex command builders),profiles.py(TOML profile resolution),progress.py(Rich live progress),run_history.py(JSONL run persistence),tui.py+tui_state.py(Textual TUI entry point and state model)README.md,CONTRIBUTING.md,SECURITY.md,CODE_OF_CONDUCT.md,SUPPORT.md,CHANGELOG.md, and governed docs underdocs/tests/test_devex_doc.pycontains a syntax error that prevents the module from importing, causing its test to be silently skippedMacroscope summarized 7cc930f.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Infrastructure
Greptile Summary
This PR packages
code-review-loop(revrem) for its first public GitHub release, adding the full source tree, packaging infrastructure, CI/CD workflows, and community governance files.src/code_review_loop/including the CLI orchestration loop, TOML profile system, harness adapter registry, Rich-based progress rendering, run history persistence, and an optional Textual TUI..github/workflows for CI (test/lint/type-check/secret-scan), a release workflow with SBOM generation and build attestation, and an OpenSSF Scorecard workflow; all Actions are SHA-pinned.scripts/install-dev,scripts/promote-stable,scripts/dev-check, pre-commit hooks) and community files (README.md,CONTRIBUTING.md,SECURITY.md,CODE_OF_CONDUCT.md, issue templates,CHANGELOG.md).Confidence Score: 5/5
Safe to merge; the core loop, subprocess teardown, and profile resolution are well-implemented with no blocking defects in the new code.
The subprocess process-group management (start_new_session=True + os.killpg) is correct, the communicate() timeout-retry pattern matches the documented Python guarantee that output is not lost, optional rich/textual dependencies are guarded with ImportError fallbacks throughout, and the only finding is a style-level duplication of a hardcoded model default string in profiles.py.
src/code_review_loop/profiles.py — duplicate hardcoded commit model default string at the dataclass field and in parse_commit; all other files are clean.
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant CLI as cli.py (main) participant Profiles as profiles.py participant Runner as default_runner participant Codex as codex subprocess participant History as run_history.py User->>CLI: revrem [--profile P] [--base B] CLI->>Profiles: resolve_profile(cwd, name) Profiles-->>CLI: Profile (LoopConfig fields) CLI->>CLI: run_loop(config) loop for iteration 1..max_iterations CLI->>Runner: run_codex_review(command) Runner->>Codex: "Popen(start_new_session=True)" Codex-->>Runner: stdout/stderr Runner-->>CLI: "CommandResult (status=clear|findings)" alt "status == clear" CLI-->>User: summary (exit 0) else findings remain CLI->>Runner: run_codex_remediation(prompt+findings) Runner->>Codex: Popen(exec, full-auto) Codex-->>Runner: stdout/stderr Runner-->>CLI: CommandResult opt commit_after_remediation CLI->>Codex: git add -A / git commit end opt check_commands CLI->>Runner: run_checks(commands) Runner-->>CLI: check results end end end CLI->>History: append_history(summary, cwd) CLI-->>User: summary JSON + exit codeComments Outside Diff (1)
.gitignore, line 34-35 (link).superpowers/development artifacts committed without being gitignoredThe
.superpowers/brainstorm/tree is included in this PR and is not covered by.gitignore. These files contain ephemeral development session state — a local server PID (872749), a localhost port binding, and server log lines that embed the developer's full local filesystem path (/home/cmf/code/code-review-loop/…), leaking the system usernamecmf. That path appears verbatim instate/server.logandstate/events. For a public launch, committing local-path data and process artifacts is a privacy concern and clutters the public history.Add
.superpowers/to.gitignoreand remove the already-committed files from the tree before the first public push.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (6): Last reviewed commit: "fix(cli): align RevRem remediation outpu..." | Re-trigger Greptile