fix: close post-merge safety gaps#3
Conversation
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe PR adds persisted self/operator merge classes, head-bound independent-review receipts, lane-scoped PR watching and merge commands, configurable branch and sandbox handling, runtime configuration migration, archive path improvements, and integration coverage for these workflows. ChangesLane governance and merge flow
Runtime configuration migration
Plan archive rebuilding
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ec4ae13 to
6cb59e8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
scripts/tests/test_pr_watch.py (1)
13-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using a pytest fixture for module loading.
Converting
_load_pr_watchinto a fixture leverages pytest idioms and reduces boilerplate, as you won't need to manually invoke it inside every test function.💡 Proposed refactor
-def _load_pr_watch() -> ModuleType: +@pytest.fixture +def pr_watch() -> ModuleType: spec = importlib.util.spec_from_file_location( "pr_watch_under_test", ENGINE_DIR / "pr_watch.py" )You can then inject
pr_watchdirectly into the test signatures (e.g.,def test_example(pr_watch: ModuleType):) and remove thepr_watch = _load_pr_watch()instantiation lines from each test.🤖 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/tests/test_pr_watch.py` around lines 13 - 20, Convert _load_pr_watch into a pytest fixture that returns the loaded pr_watch module, then inject the pr_watch fixture into each test that currently calls _load_pr_watch and remove those manual instantiation lines.scripts/archive_plan_sessions.py (1)
262-298: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify absolute path resolution.
The
pathlib.Pathdivision operator/automatically discards the left operand if the right operand is already an absolute path. The ternary checks foris_absolute()are redundant and can be safely removed.♻️ Proposed refactor
- args.plan = args.plan if args.plan.is_absolute() else Path.cwd() / args.plan - args.history = ( - args.history if args.history.is_absolute() else Path.cwd() / args.history - ) + args.plan = Path.cwd() / args.plan + args.history = Path.cwd() / args.history🤖 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/archive_plan_sessions.py` around lines 262 - 298, Remove the redundant is_absolute() ternary checks when normalizing args.plan and args.history in the argument-parsing flow. Construct both paths directly with Path.cwd() / the configured argument, relying on pathlib to preserve already-absolute paths.
🤖 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 `@docs/agentic-dev-kit/workflows/pr-watch.md`:
- Around line 87-88: Revise the workflow documentation statement about platform
approval so it no longer implies that --record-review records an APPROVED state.
Clarify that approval must be explicitly receipted against the reviewed head
SHA, preserving the requirement that later pushes cannot invalidate an untracked
approval.
In `@docs/autonomous-session-playbook.md`:
- Around line 87-90: Update the non-lane/operator merge instructions to require
an immediate poll confirming done: true, capture its validated head commit, and
pass that head to gh pr merge via --match-head-commit; preserve the existing
squash and branch-deletion options.
In `@docs/parallel-howto.md`:
- Line 95: Update the command examples for scripts/dev_session.sh to use one
concrete valid --merge-class value instead of the angle-bracket placeholder, and
briefly explain that the alternative value is operator. Apply the same
correction to the other occurrence.
In `@init.sh`:
- Around line 162-183: Update append_to_section to return failure when the
target section header is not found or the block is not inserted, while
preserving its existing insertion behavior for matched sections. In
migrate_runtime_schema, check append_to_section’s return status at each call
site before printing the corresponding success message, and propagate or report
failure instead of claiming migration succeeded.
- Around line 206-236: Update the generated YAML in the fallback_commands and
runtime_mappings blocks to quote every interpolated legacy scalar: old_fallback,
old_cheap, old_default, and old_expensive. Keep the existing defaults and
mappings unchanged, but ensure these values are emitted as valid YAML scalars
consistently with the quoted codex fallback.
In `@scripts/archive_plan_sessions.py`:
- Around line 126-143: Update split_plan to include introductory text after
RECENT_SESSIONS_HEADING in head, stopping at the first session block rather than
immediately after the heading, so parse_blocks cannot discard it. Update
rebuild_plan to locate and validate RECENT_SESSIONS_HEADING anywhere in head
instead of assuming head[-1] is the heading.
In `@scripts/dev_session.sh`:
- Line 730: Rename the done variable to a non-keyword identifier such as
pr_ready throughout the surrounding flow: its local declaration, read
assignment, and all references on the affected lines. Preserve the existing
readiness logic and behavior.
In `@scripts/pr_watch.py`:
- Around line 453-457: Update save_state and the related state read-modify-write
flows in persist_poll and record_review to use a cross-process lock covering the
complete transaction, including state reads, receipt updates, and writes.
Replace direct write_text persistence with a same-directory temporary-file write
followed by os.replace so readers never observe partial state, and apply the
same atomic persistence behavior to all referenced save paths.
In `@scripts/tests/test_portability.py`:
- Around line 373-395: Update
test_operator_merge_class_refuses_before_contacting_github to place a sentinel
gh executable earlier in PATH that records invocation, while preserving the
existing restricted-tool setup. After running dev_session.sh merge, assert the
sentinel marker was not created, in addition to the current failure and
operator-merge assertions.
- Around line 841-849: Extend the configuration assertions in the portability
test to verify that the migrated runtime entry preserves the fixture’s legacy
fallback command “/code-review”, alongside the existing Codex fallback
assertion. Use the relevant runtime fallback-command symbol already covered by
the surrounding assertions and ensure replacement of the supplied legacy value
causes the test to fail.
---
Nitpick comments:
In `@scripts/archive_plan_sessions.py`:
- Around line 262-298: Remove the redundant is_absolute() ternary checks when
normalizing args.plan and args.history in the argument-parsing flow. Construct
both paths directly with Path.cwd() / the configured argument, relying on
pathlib to preserve already-absolute paths.
In `@scripts/tests/test_pr_watch.py`:
- Around line 13-20: Convert _load_pr_watch into a pytest fixture that returns
the loaded pr_watch module, then inject the pr_watch fixture into each test that
currently calls _load_pr_watch and remove those manual instantiation 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: 567f7be8-8c65-4cd1-988d-26d4949332c5
📒 Files selected for processing (14)
README.mddocs/agentic-dev-kit/workflows/parallel.mddocs/agentic-dev-kit/workflows/pr-watch.mddocs/autonomous-session-playbook.mddocs/parallel-dev.mddocs/parallel-howto.mdinit.shscripts/archive_plan_sessions.pyscripts/dev_session.shscripts/lib/repo_root.shscripts/pr_watch.pyscripts/reconcile_sessions.shscripts/tests/test_portability.pyscripts/tests/test_pr_watch.py
| sign-off. For non-lane/operator merges: | ||
| ```sh | ||
| gh pr merge <PR#> --squash --delete-branch | ||
| git checkout <protected_branch> && git pull --ff-only origin <protected_branch> |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Pin direct terminal merges to the validated head.
This command can merge a push that lands after the final review poll. Require an immediate done: true poll and pass its head through --match-head-commit.
Proposed fix
-gh pr merge <PR#> --squash --delete-branch
+gh pr merge <PR#> --squash --delete-branch --match-head-commit <validated-head>📝 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.
| sign-off. For non-lane/operator merges: | |
| ```sh | |
| gh pr merge <PR#> --squash --delete-branch | |
| git checkout <protected_branch> && git pull --ff-only origin <protected_branch> | |
| sign-off. For non-lane/operator merges: |
🤖 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/autonomous-session-playbook.md` around lines 87 - 90, Update the
non-lane/operator merge instructions to require an immediate poll confirming
done: true, capture its validated head commit, and pass that head to gh pr merge
via --match-head-commit; preserve the existing squash and branch-deletion
options.
| append_to_section() { | ||
| section="$1" | ||
| block="$2" | ||
| tmpfile="${CONFIG_FILE}.tmp.$$" | ||
| blockfile="${tmpfile}.block" | ||
| printf '%s\n' "$block" > "$blockfile" | ||
| awk -v section="$section" -v blockfile="$blockfile" ' | ||
| function emit( line) { | ||
| while ((getline line < blockfile) > 0) print line | ||
| close(blockfile) | ||
| } | ||
| $0 == section { inside = 1 } | ||
| inside && $0 != section && $0 ~ /^[A-Za-z_][A-Za-z0-9_]*:/ && !inserted { | ||
| emit() | ||
| inserted = 1 | ||
| inside = 0 | ||
| } | ||
| { print } | ||
| END { if (inside && !inserted) emit() } | ||
| ' "$CONFIG_FILE" > "$tmpfile" && mv "$tmpfile" "$CONFIG_FILE" | ||
| rm -f "$blockfile" | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
append_to_section silently no-ops (and migrate_runtime_schema reports false success) when the target section header isn't found exactly.
append_to_section's END fallback only fires if (inside && !inserted) — i.e. only when the section header was matched but no later top-level key existed. If $0 == section never matches at all (e.g. paths:, review:, or models: has trailing whitespace/a tab, or the section is genuinely missing), inside stays 0, the block is silently dropped, and the file is written back unchanged. Yet every call site in migrate_runtime_schema (Lines 193, 212, 235) unconditionally prints "added ... to config/dev-model.yaml" right after the call, regardless of whether anything was actually inserted. This contradicts the PR's stated goal of deterministic, fail-closed migration — a malformed/legacy config could permanently fail to gain paths.engines, review.fallback_commands, or models.tiers while every future init.sh run keeps claiming success.
Contrast with insert_before_section, which matches the anchor via a prefix check (index($0, anchor) == 1) and has a real EOF fallback that always fires — so it can't silently drop its block the same way.
Suggested fix: make append_to_section report failure, and check it before echoing success
append_to_section() {
section="$1"
block="$2"
tmpfile="${CONFIG_FILE}.tmp.$$"
blockfile="${tmpfile}.block"
printf '%s\n' "$block" > "$blockfile"
- awk -v section="$section" -v blockfile="$blockfile" '
+ status=0
+ awk -v section="$section" -v blockfile="$blockfile" '
function emit( line) {
while ((getline line < blockfile) > 0) print line
close(blockfile)
}
$0 == section { inside = 1 }
inside && $0 != section && $0 ~ /^[A-Za-z_][A-Za-z0-9_]*:/ && !inserted {
emit()
inserted = 1
inside = 0
}
{ print }
- END { if (inside && !inserted) emit() }
+ END {
+ if (inside && !inserted) emit()
+ if (!inserted && !inside) exit 1
+ }
' "$CONFIG_FILE" > "$tmpfile" && mv "$tmpfile" "$CONFIG_FILE" || status=1
rm -f "$blockfile"
+ return "$status"
}and in migrate_runtime_schema:
if ! grep -q '^ engines:' "$CONFIG_FILE"; then
- append_to_section "paths:" ' # Directory containing the deterministic kit engines.
- engines: scripts'
- echo "added paths.engines to config/dev-model.yaml"
+ if append_to_section "paths:" ' # Directory containing the deterministic kit engines.
+ engines: scripts'; then
+ echo "added paths.engines to config/dev-model.yaml"
+ else
+ echo "warning: could not locate 'paths:' section; skipped engines migration" >&2
+ fi
fiAlso applies to: 189-237
🤖 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 `@init.sh` around lines 162 - 183, Update append_to_section to return failure
when the target section header is not found or the block is not inserted, while
preserving its existing insertion behavior for matched sections. In
migrate_runtime_schema, check append_to_section’s return status at each call
site before printing the corresponding success message, and propagate or report
failure instead of claiming migration succeeded.
| if ! grep -q '^ fallback_commands:' "$CONFIG_FILE"; then | ||
| old_fallback=$(get_field "review:" "" "^ fallback_command:") | ||
| [ -n "$old_fallback" ] || old_fallback="/code-review" | ||
| append_to_section "review:" " fallback_commands: | ||
| claude: $old_fallback | ||
| codex: \"/review\"" | ||
| echo "added runtime review fallbacks to config/dev-model.yaml" | ||
| fi | ||
|
|
||
| if ! grep -q '^ tiers:' "$CONFIG_FILE"; then | ||
| old_cheap=$(get_field "models:" "" "^ cheap:") | ||
| old_default=$(get_field "models:" "" "^ default:") | ||
| old_expensive=$(get_field "models:" "" "^ expensive:") | ||
| [ -n "$old_cheap" ] || old_cheap="haiku" | ||
| [ -n "$old_default" ] || old_default="sonnet" | ||
| [ -n "$old_expensive" ] || old_expensive="opus" | ||
| append_to_section "models:" " tiers: | ||
| cheap: mechanical | ||
| default: standard | ||
| expensive: judgment | ||
| runtime_mappings: | ||
| claude: | ||
| cheap: $old_cheap | ||
| default: $old_default | ||
| expensive: $old_expensive | ||
| codex: | ||
| cheap: low | ||
| default: medium | ||
| expensive: high" | ||
| echo "added runtime model mappings to config/dev-model.yaml" | ||
| fi |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Inconsistent quoting of interpolated legacy values risks malformed YAML.
$old_fallback (Line 210) and $old_cheap/$old_default/$old_expensive (Lines 228-230) are interpolated unquoted into the generated YAML, while the codex default is explicitly quoted (codex: "/review", Line 211). If a legacy fallback_command/model value contains a # (YAML comment start after whitespace), a leading special character, or a colon, the unquoted substitution can produce truncated or invalid YAML that yaml.safe_load will misparse or reject.
Suggested fix: quote all interpolated legacy scalars consistently
append_to_section "review:" " fallback_commands:
- claude: $old_fallback
+ claude: \"$old_fallback\"
codex: \"/review\"" codex:
cheap: $old_cheap append_to_section "models:" " tiers:
cheap: mechanical
default: standard
expensive: judgment
runtime_mappings:
claude:
- cheap: $old_cheap
- default: $old_default
- expensive: $old_expensive
+ cheap: \"$old_cheap\"
+ default: \"$old_default\"
+ expensive: \"$old_expensive\"📝 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.
| if ! grep -q '^ fallback_commands:' "$CONFIG_FILE"; then | |
| old_fallback=$(get_field "review:" "" "^ fallback_command:") | |
| [ -n "$old_fallback" ] || old_fallback="/code-review" | |
| append_to_section "review:" " fallback_commands: | |
| claude: $old_fallback | |
| codex: \"/review\"" | |
| echo "added runtime review fallbacks to config/dev-model.yaml" | |
| fi | |
| if ! grep -q '^ tiers:' "$CONFIG_FILE"; then | |
| old_cheap=$(get_field "models:" "" "^ cheap:") | |
| old_default=$(get_field "models:" "" "^ default:") | |
| old_expensive=$(get_field "models:" "" "^ expensive:") | |
| [ -n "$old_cheap" ] || old_cheap="haiku" | |
| [ -n "$old_default" ] || old_default="sonnet" | |
| [ -n "$old_expensive" ] || old_expensive="opus" | |
| append_to_section "models:" " tiers: | |
| cheap: mechanical | |
| default: standard | |
| expensive: judgment | |
| runtime_mappings: | |
| claude: | |
| cheap: $old_cheap | |
| default: $old_default | |
| expensive: $old_expensive | |
| codex: | |
| cheap: low | |
| default: medium | |
| expensive: high" | |
| echo "added runtime model mappings to config/dev-model.yaml" | |
| fi | |
| if ! grep -q '^ fallback_commands:' "$CONFIG_FILE"; then | |
| old_fallback=$(get_field "review:" "" "^ fallback_command:") | |
| [ -n "$old_fallback" ] || old_fallback="/code-review" | |
| append_to_section "review:" " fallback_commands: | |
| claude: \"$old_fallback\" | |
| codex: \"/review\"" | |
| echo "added runtime review fallbacks to config/dev-model.yaml" | |
| fi | |
| if ! grep -q '^ tiers:' "$CONFIG_FILE"; then | |
| old_cheap=$(get_field "models:" "" "^ cheap:") | |
| old_default=$(get_field "models:" "" "^ default:") | |
| old_expensive=$(get_field "models:" "" "^ expensive:") | |
| [ -n "$old_cheap" ] || old_cheap="haiku" | |
| [ -n "$old_default" ] || old_default="sonnet" | |
| [ -n "$old_expensive" ] || old_expensive="opus" | |
| append_to_section "models:" " tiers: | |
| cheap: mechanical | |
| default: standard | |
| expensive: judgment | |
| runtime_mappings: | |
| claude: | |
| cheap: \"$old_cheap\" | |
| default: \"$old_default\" | |
| expensive: \"$old_expensive\" | |
| codex: | |
| cheap: low | |
| default: medium | |
| expensive: high" | |
| echo "added runtime model mappings to config/dev-model.yaml" | |
| 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 `@init.sh` around lines 206 - 236, Update the generated YAML in the
fallback_commands and runtime_mappings blocks to quote every interpolated legacy
scalar: old_fallback, old_cheap, old_default, and old_expensive. Keep the
existing defaults and mappings unchanged, but ensure these values are emitted as
valid YAML scalars consistently with the quoted codex fallback.
| (i for i, ln in enumerate(lines) if ln.rstrip("\n") == RECENT_SESSIONS_HEADING), | ||
| None, | ||
| ) | ||
| if recent_start is None: | ||
| raise ValueError( | ||
| "no session blocks or '## Recent sessions' section found in handoff doc" | ||
| ) | ||
| standing = next( | ||
| ( | ||
| i | ||
| for i, ln in enumerate(lines) | ||
| if i > recent_start and ln.startswith("## ") | ||
| ), | ||
| (i for i, ln in enumerate(lines) if i > recent_start and ln.startswith("## ")), | ||
| len(lines), | ||
| ) | ||
| # Keep the section heading in the head; parse_blocks handles its ``###`` | ||
| # entries and rebuild_plan preserves this layout without adding a pointer. | ||
| return lines[: recent_start + 1], lines[recent_start + 1 : standing], lines[standing:] | ||
| return ( | ||
| lines[: recent_start + 1], | ||
| lines[recent_start + 1 : standing], | ||
| lines[standing:], | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve introductory text under ## Recent sessions.
Currently, split_plan terminates head immediately after the ## Recent sessions heading line. Any subsequent text before the first ### YYYY-MM-DD block is assigned to region but silently dropped by parse_blocks (since cur remains None until the first heading). This will cause unintended data loss if users manually add notes below the section heading.
Include everything up to the first block in head to preserve it, and update rebuild_plan (which currently assumes head[-1] is exactly the heading) to check if the heading exists anywhere in head.
🛡️ Proposed fixes
Apply this fix to split_plan to expand head up to the first session block:
recent_start = next(
(i for i, ln in enumerate(lines) if ln.rstrip("\n") == RECENT_SESSIONS_HEADING),
None,
)
if recent_start is None:
raise ValueError(
"no session blocks or '## Recent sessions' section found in handoff doc"
)
standing = next(
(i for i, ln in enumerate(lines) if i > recent_start and ln.startswith("## ")),
len(lines),
)
- # Keep the section heading in the head; parse_blocks handles its ``###``
- # entries and rebuild_plan preserves this layout without adding a pointer.
+
+ first_block = next(
+ (i for i, ln in enumerate(lines) if i > recent_start and bool(_RECENT_SESSION_RE.match(ln))),
+ standing,
+ )
+
+ # Include everything up to the first session block in the head to preserve introductory text.
return (
- lines[: recent_start + 1],
- lines[recent_start + 1 : standing],
+ lines[:first_block],
+ lines[first_block:standing],
lines[standing:],
)Also, apply this corresponding check in rebuild_plan to support the updated head shape:
def rebuild_plan(
head: list[str],
keep_blocks: list[list[str]],
tail: list[str],
keep: int,
*,
history_link: str = "handoff-history.md",
history_label: str = "handoff-history.md",
) -> list[str]:
"""Reassemble the handoff doc from the trimmed head, kept blocks, fresh pointer, and tail."""
- if head and head[-1].rstrip("\n") == RECENT_SESSIONS_HEADING:
+ if any(ln.rstrip("\n") == RECENT_SESSIONS_HEADING for ln in head):
body: list[str] = ["\n"]📝 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.
| (i for i, ln in enumerate(lines) if ln.rstrip("\n") == RECENT_SESSIONS_HEADING), | |
| None, | |
| ) | |
| if recent_start is None: | |
| raise ValueError( | |
| "no session blocks or '## Recent sessions' section found in handoff doc" | |
| ) | |
| standing = next( | |
| ( | |
| i | |
| for i, ln in enumerate(lines) | |
| if i > recent_start and ln.startswith("## ") | |
| ), | |
| (i for i, ln in enumerate(lines) if i > recent_start and ln.startswith("## ")), | |
| len(lines), | |
| ) | |
| # Keep the section heading in the head; parse_blocks handles its ``###`` | |
| # entries and rebuild_plan preserves this layout without adding a pointer. | |
| return lines[: recent_start + 1], lines[recent_start + 1 : standing], lines[standing:] | |
| return ( | |
| lines[: recent_start + 1], | |
| lines[recent_start + 1 : standing], | |
| lines[standing:], | |
| ) | |
| (i for i, ln in enumerate(lines) if ln.rstrip("\n") == RECENT_SESSIONS_HEADING), | |
| None, | |
| ) | |
| if recent_start is None: | |
| raise ValueError( | |
| "no session blocks or '## Recent sessions' section found in handoff doc" | |
| ) | |
| standing = next( | |
| (i for i, ln in enumerate(lines) if i > recent_start and ln.startswith("## ")), | |
| len(lines), | |
| ) | |
| first_block = next( | |
| (i for i, ln in enumerate(lines) if i > recent_start and bool(_RECENT_SESSION_RE.match(ln))), | |
| standing, | |
| ) | |
| # Include everything up to the first session block in the head to preserve introductory text. | |
| return ( | |
| lines[:first_block], | |
| lines[first_block:standing], | |
| lines[standing:], | |
| ) |
🤖 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/archive_plan_sessions.py` around lines 126 - 143, Update split_plan
to include introductory text after RECENT_SESSIONS_HEADING in head, stopping at
the first session block rather than immediately after the heading, so
parse_blocks cannot discard it. Update rebuild_plan to locate and validate
RECENT_SESSIONS_HEADING anywhere in head instead of assuming head[-1] is the
heading.
|
|
||
| local merge_class="operator" branch="" base="$DEFAULT_BASE" | ||
| local resolved repo_nwo pr | ||
| local report done validated_pr validated_base validated_head |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Rename the done local variable to avoid the reserved-word collision.
Shellcheck flags lines 730 and 748 (SC1010) because done is a bash keyword being used as a plain identifier (local report done ... / read -r done ...). It's functionally valid here, but confusing and triggers CI's static-analysis noise flagged in this PR's own hints. A quick rename (e.g. pr_ready) resolves the warnings and improves readability.
♻️ Proposed rename
- local report done validated_pr validated_base validated_head
+ local report pr_ready validated_pr validated_base validated_head
[[ -s "$session_dir/merge_class" ]] && merge_class="$(cat "$session_dir/merge_class")"
@@
- IFS=$'\t' read -r done validated_pr validated_base validated_head <<< "$(printf '%s' "$report" | python3 -c '
+ IFS=$'\t' read -r pr_ready validated_pr validated_base validated_head <<< "$(printf '%s' "$report" | python3 -c '
import json, sys
d = json.load(sys.stdin)
print("\t".join(("true" if d.get("done") is True else "false", str(d.get("pr") or ""), str(d.get("base") or ""), str(d.get("head") or ""))))
')"
- [[ "$done" == "true" ]] \
+ [[ "$pr_ready" == "true" ]] \
|| _die "PR #$pr is not green, review-clean, and merge-ready; run pr-watch to convergence first"Also applies to: 748-748, 751-751, 753-753
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 730-730: Use semicolon or linefeed before 'done' (or quote to make it literal).
(SC1010)
🤖 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_session.sh` at line 730, Rename the done variable to a
non-keyword identifier such as pr_ready throughout the surrounding flow: its
local declaration, read assignment, and all references on the affected lines.
Preserve the existing readiness logic and behavior.
Source: Linters/SAST tools
| def save_state(pr: int, state: dict) -> None: | ||
| STATE_DIR.mkdir(parents=True, exist_ok=True) | ||
| _seen_path(pr).write_text(json.dumps(state, indent=1, sort_keys=True), encoding="utf-8") | ||
| _seen_path(pr).write_text( | ||
| json.dumps(state, indent=1, sort_keys=True), encoding="utf-8" | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Serialize and atomically commit PR-state updates.
persist_poll() can load state before record_review() writes its receipt, then save afterward and delete that new receipt. Direct write_text() also permits readers to observe a partial file. Use a cross-process lock around each read-modify-write transaction and write via temporary file plus os.replace().
Also applies to: 535-537, 760-763
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 455-455: use jsonify instead of json.dumps for JSON output
Context: json.dumps(state, indent=1, sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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/pr_watch.py` around lines 453 - 457, Update save_state and the
related state read-modify-write flows in persist_poll and record_review to use a
cross-process lock covering the complete transaction, including state reads,
receipt updates, and writes. Replace direct write_text persistence with a
same-directory temporary-file write followed by os.replace so readers never
observe partial state, and apply the same atomic persistence behavior to all
referenced save paths.
| def test_operator_merge_class_refuses_before_contacting_github(tmp_path: Path) -> None: | ||
| repo, engine_dir, sessions = _install_real_trunk_repo(tmp_path) | ||
| env = {**os.environ, "DEVKIT_SESSIONS_DIR": str(sessions)} | ||
| subprocess.run( | ||
| ["bash", str(engine_dir / "dev_session.sh"), "new", "probe", "--headless"], | ||
| cwd=repo, | ||
| env=env, | ||
| check=True, | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
|
|
||
| result = subprocess.run( | ||
| ["bash", str(engine_dir / "dev_session.sh"), "merge", "probe"], | ||
| cwd=repo, | ||
| env={**env, "PATH": "/usr/bin:/bin"}, | ||
| check=False, | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
|
|
||
| assert result.returncode != 0 | ||
| assert "operator-merge" in result.stderr |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Actually assert that GitHub is never invoked.
The test name promises refusal before GitHub contact, but no fake gh records or rejects invocation. A regression could contact GitHub and still emit the expected later error. Install a sentinel gh executable and assert its marker remains absent.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 375-382: Command coming from incoming request
Context: subprocess.run(
["bash", str(engine_dir / "dev_session.sh"), "new", "probe", "--headless"],
cwd=repo,
env=env,
check=True,
capture_output=True,
text=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 384-391: Command coming from incoming request
Context: subprocess.run(
["bash", str(engine_dir / "dev_session.sh"), "merge", "probe"],
cwd=repo,
env={**env, "PATH": "/usr/bin:/bin"},
check=False,
capture_output=True,
text=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.15.21)
[error] 376-376: subprocess call: check for execution of untrusted input
(S603)
[error] 377-377: Starting a process with a partial executable path
(S607)
[error] 385-385: subprocess call: check for execution of untrusted input
(S603)
[error] 386-386: Starting a process with a partial executable path
(S607)
🤖 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/tests/test_portability.py` around lines 373 - 395, Update
test_operator_merge_class_refuses_before_contacting_github to place a sentinel
gh executable earlier in PATH that records invocation, while preserving the
existing restricted-tool setup. After running dev_session.sh merge, assert the
sentinel marker was not created, in addition to the current failure and
operator-merge assertions.
| assert config["paths"]["engines"] == "scripts" | ||
| assert config["runtime"]["default"] == "claude" | ||
| assert config["runtime"]["launchers"]["codex"] == "codex" | ||
| assert config["review"]["fallback_commands"]["codex"] == "/review" | ||
| assert config["models"]["runtime_mappings"]["claude"] == { | ||
| "cheap": "tiny", | ||
| "default": "normal", | ||
| "expensive": "large", | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Assert preservation of the legacy fallback command.
The fixture supplies fallback_command: "/code-review", but the assertions only check the new Codex default. Add an assertion for the migrated runtime entry so replacement of an existing value fails this test.
Suggested assertion
assert config["review"]["fallback_commands"]["codex"] == "/review"
+assert config["review"]["fallback_commands"]["claude"] == "/code-review"📝 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.
| assert config["paths"]["engines"] == "scripts" | |
| assert config["runtime"]["default"] == "claude" | |
| assert config["runtime"]["launchers"]["codex"] == "codex" | |
| assert config["review"]["fallback_commands"]["codex"] == "/review" | |
| assert config["models"]["runtime_mappings"]["claude"] == { | |
| "cheap": "tiny", | |
| "default": "normal", | |
| "expensive": "large", | |
| } | |
| assert config["paths"]["engines"] == "scripts" | |
| assert config["runtime"]["default"] == "claude" | |
| assert config["runtime"]["launchers"]["codex"] == "codex" | |
| assert config["review"]["fallback_commands"]["codex"] == "/review" | |
| assert config["review"]["fallback_commands"]["claude"] == "/code-review" | |
| assert config["models"]["runtime_mappings"]["claude"] == { | |
| "cheap": "tiny", | |
| "default": "normal", | |
| "expensive": "large", | |
| } |
🤖 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/tests/test_portability.py` around lines 841 - 849, Extend the
configuration assertions in the portability test to verify that the migrated
runtime entry preserves the fixture’s legacy fallback command “/code-review”,
alongside the existing Codex fallback assertion. Use the relevant runtime
fallback-command symbol already covered by the surrounding assertions and ensure
replacement of the supplied legacy value causes the test to fail.
1 participant
Summary
Safety evidence
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores