[limen RESOLVE-organvm-i-theoria-.github-458] resolve .github#458 (BLOCKED) - #463
[limen RESOLVE-organvm-i-theoria-.github-458] resolve .github#458 (BLOCKED)#4634444J99 wants to merge 1 commit into
Conversation
limen task RESOLVE-organvm-i-theoria-.github-458
Reviewer's GuideRefactors the staggered scheduling GitHub Actions workflow to delegate schedule planning and execution logic to a new reusable Python helper, adds tests, and passes richer metadata (date, stagger interval, default branches) through job outputs to make schedule reuse and completion more robust. Sequence diagram for staggered scheduling workflow using Python helpersequenceDiagram
participant GitHubActions
participant calculate_schedule as Job_calculate_schedule
participant script as staggered_walkthrough_schedule_py
participant execute_schedule as Job_execute_schedule
participant GitHubAPI
GitHubActions->>calculate_schedule: start job
calculate_schedule->>script: staggered_walkthrough_schedule.py plan
script-->>calculate_schedule: has_schedule, schedule_days, total_repositories
calculate_schedule->>script: staggered_walkthrough_schedule.py due
script-->>calculate_schedule: repos_today, schedule_date, stagger_minutes
calculate_schedule-->>GitHubActions: outputs repos_today, schedule_date, stagger_minutes
GitHubActions->>execute_schedule: start job (needs.calculate_schedule.outputs)
execute_schedule->>GitHubAPI: GET /repos/{repo}
GitHubAPI-->>execute_schedule: { default_branch }
execute_schedule->>GitHubAPI: POST /repos/{repo}/actions/workflows/generate-walkthrough.yml/dispatches (ref=default_branch)
execute_schedule-->>GitHubActions: staggered execution using STAGGER_MINUTES
GitHubActions->>execute_schedule: always() Update schedule status step
execute_schedule->>script: staggered_walkthrough_schedule.py complete
script-->>execute_schedule: updated schedule.json
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
🤖 Hi @4444J99, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
Version Control Standards Validation FailedThis pull request does not meet our version control standards. Common Issues:
Documentation: Please update your branch name and/or commit messages to follow the standards. |
|
🔍 Reviewers Assigned Reviewers have been automatically assigned based on the CODEOWNERS file. What's Next:
Need Help? Automated reviewer assignment - PR #463 |
Action Pinning RequiredThis PR contains GitHub Actions that are not pinned to SHA commits. Why this matters: How to fix:
Example: # Before (vulnerable)
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # ratchet:actions/checkout@v6.0.2
# After (secure)
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # ratchet:actions/checkout@v6.0.2 |
github-actions
Bot
added
github-actions
💬 Task Catcher Summary💬 Unresolved discussions - Resolve review threads 📋 Task Overview
🎯 Next Steps
Options:
Last scanned: 2026-07-19 08:42 UTC |
Version Control Standards Validation FailedThis pull request does not meet our version control standards. Common Issues:
Documentation: Please update your branch name and/or commit messages to follow the standards. |
|
🤖 I'm sorry @4444J99, but I was unable to process your request. Please see the logs for more details. |
🧪 Integration Test Results
Overall: ✅ All tests passed! View detailed test reports |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?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 introduces a new Python script (staggered_walkthrough_schedule.py) and corresponding unit tests to manage staggered walkthrough schedules for repositories. The reviewer's feedback focuses on improving robustness and platform compatibility. Key recommendations include explicitly specifying encoding='utf-8' when reading and writing files, adding defensive type checking for repository inputs, fixing a bug where a 0-minute stagger is incorrectly overridden by the default value, and leveraging argparse built-in type validation to eliminate redundant runtime type casting.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def load_json(path: Path) -> Any: | ||
| """Load JSON from a path.""" | ||
| with path.open() as file: | ||
| return json.load(file) |
There was a problem hiding this comment.
Specify encoding="utf-8" explicitly when opening files to prevent platform-dependent encoding issues (e.g., on Windows runners where the default encoding might not be UTF-8).
| def load_json(path: Path) -> Any: | |
| """Load JSON from a path.""" | |
| with path.open() as file: | |
| return json.load(file) | |
| def load_json(path: Path) -> Any: | |
| """Load JSON from a path.""" | |
| with path.open(encoding="utf-8") as file: | |
| return json.load(file) |
| def write_json(path: Path, data: Any) -> None: | ||
| """Write indented JSON to a path.""" | ||
| path.parent.mkdir(parents=True, exist_ok=True) | ||
| with path.open("w") as file: | ||
| json.dump(data, file, indent=2) | ||
| file.write("\n") |
There was a problem hiding this comment.
Specify encoding="utf-8" explicitly when writing JSON files to ensure consistent UTF-8 encoding across all environments.
| def write_json(path: Path, data: Any) -> None: | |
| """Write indented JSON to a path.""" | |
| path.parent.mkdir(parents=True, exist_ok=True) | |
| with path.open("w") as file: | |
| json.dump(data, file, indent=2) | |
| file.write("\n") | |
| def write_json(path: Path, data: Any) -> None: | |
| """Write indented JSON to a path.""" | |
| path.parent.mkdir(parents=True, exist_ok=True) | |
| with path.open("w", encoding="utf-8") as file: | |
| json.dump(data, file, indent=2) | |
| file.write("\n") |
| def normalize_repo(repo: dict[str, Any]) -> dict[str, Any]: | ||
| """Normalize repository metadata used by the scheduler.""" | ||
| full_name = str(repo.get("full_name") or repo.get("name") or "").strip() | ||
| if not full_name: | ||
| raise ValueError(f"Repository entry is missing full_name/name: {repo}") |
There was a problem hiding this comment.
Add defensive type checking to ensure repo is a dictionary before calling .get(). This prevents unhandled AttributeError exceptions if invalid repository metadata is passed.
| def normalize_repo(repo: dict[str, Any]) -> dict[str, Any]: | |
| """Normalize repository metadata used by the scheduler.""" | |
| full_name = str(repo.get("full_name") or repo.get("name") or "").strip() | |
| if not full_name: | |
| raise ValueError(f"Repository entry is missing full_name/name: {repo}") | |
| def normalize_repo(repo: dict[str, Any]) -> dict[str, Any]: | |
| """Normalize repository metadata used by the scheduler.""" | |
| if not isinstance(repo, dict): | |
| raise ValueError(f"Repository entry must be a dictionary, got {type(repo).__name__}") | |
| full_name = str(repo.get("full_name") or repo.get("name") or "").strip() | |
| if not full_name: | |
| raise ValueError(f"Repository entry is missing full_name/name: {repo}") |
| selected_date = min(due_dates).isoformat() | ||
| entry = schedule["schedule"][selected_date] | ||
| repositories = [str(repo) for repo in entry.get("repositories", [])] | ||
| stagger_minutes = int(entry.get("stagger_minutes") or DEFAULT_STAGGER_MINUTES) |
There was a problem hiding this comment.
Using or on entry.get("stagger_minutes") will override a valid stagger time of 0 to the default 5 minutes because 0 is falsy in Python. Check for None explicitly to allow a 0 minute stagger.
| selected_date = min(due_dates).isoformat() | |
| entry = schedule["schedule"][selected_date] | |
| repositories = [str(repo) for repo in entry.get("repositories", [])] | |
| stagger_minutes = int(entry.get("stagger_minutes") or DEFAULT_STAGGER_MINUTES) | |
| selected_date = min(due_dates).isoformat() | |
| entry = schedule["schedule"][selected_date] | |
| repositories = [str(repo) for repo in entry.get("repositories", [])] | |
| stagger_val = entry.get("stagger_minutes") | |
| stagger_minutes = int(stagger_val) if stagger_val is not None else DEFAULT_STAGGER_MINUTES |
| def write_github_output(values: dict[str, Any]) -> None: | ||
| """Append simple key/value outputs for GitHub Actions.""" | ||
| output_path = os.getenv("GITHUB_OUTPUT") | ||
| if not output_path: | ||
| return | ||
| with open(output_path, "a") as file: | ||
| for key, value in values.items(): | ||
| file.write(f"{key}={value}\n") |
There was a problem hiding this comment.
Specify encoding="utf-8" when appending to the GitHub Actions output file to prevent encoding issues with special characters.
| def write_github_output(values: dict[str, Any]) -> None: | |
| """Append simple key/value outputs for GitHub Actions.""" | |
| output_path = os.getenv("GITHUB_OUTPUT") | |
| if not output_path: | |
| return | |
| with open(output_path, "a") as file: | |
| for key, value in values.items(): | |
| file.write(f"{key}={value}\n") | |
| def write_github_output(values: dict[str, Any]) -> None: | |
| """Append simple key/value outputs for GitHub Actions.""" | |
| output_path = os.getenv("GITHUB_OUTPUT") | |
| if not output_path: | |
| return | |
| with open(output_path, "a", encoding="utf-8") as file: | |
| for key, value in values.items(): | |
| file.write(f"{key}={value}\n") |
| today = parse_iso_date(args.today) | ||
| repos_per_day = int(args.repos_per_day) | ||
| force = parse_bool(args.force) | ||
|
|
||
| schedule, regenerated, reason = plan_schedule( | ||
| repositories, | ||
| existing_schedule, | ||
| repos_per_day, | ||
| today, | ||
| force=force, | ||
| stagger_minutes=int(args.stagger_minutes), | ||
| ) |
There was a problem hiding this comment.
If argparse is configured to parse --repos-per-day and --stagger-minutes as integers directly, we can remove the redundant int() type casts here.
| today = parse_iso_date(args.today) | |
| repos_per_day = int(args.repos_per_day) | |
| force = parse_bool(args.force) | |
| schedule, regenerated, reason = plan_schedule( | |
| repositories, | |
| existing_schedule, | |
| repos_per_day, | |
| today, | |
| force=force, | |
| stagger_minutes=int(args.stagger_minutes), | |
| ) | |
| today = parse_iso_date(args.today) | |
| repos_per_day = args.repos_per_day | |
| force = parse_bool(args.force) | |
| schedule, regenerated, reason = plan_schedule( | |
| repositories, | |
| existing_schedule, | |
| repos_per_day, | |
| today, | |
| force=force, | |
| stagger_minutes=args.stagger_minutes, | |
| ) |
| plan.add_argument("--output", required=True) | ||
| plan.add_argument("--repos-per-day", required=True) | ||
| plan.add_argument("--today") | ||
| plan.add_argument("--force", default="false") | ||
| plan.add_argument("--stagger-minutes", default=str(DEFAULT_STAGGER_MINUTES)) |
There was a problem hiding this comment.
Leverage argparse built-in type validation by specifying type=int for --repos-per-day and --stagger-minutes. This provides automatic validation and user-friendly error messages on invalid inputs instead of throwing tracebacks at runtime.
| plan.add_argument("--output", required=True) | |
| plan.add_argument("--repos-per-day", required=True) | |
| plan.add_argument("--today") | |
| plan.add_argument("--force", default="false") | |
| plan.add_argument("--stagger-minutes", default=str(DEFAULT_STAGGER_MINUTES)) | |
| plan.add_argument("--output", required=True) | |
| plan.add_argument("--repos-per-day", type=int, required=True) | |
| plan.add_argument("--today") | |
| plan.add_argument("--force", default="false") | |
| plan.add_argument("--stagger-minutes", type=int, default=DEFAULT_STAGGER_MINUTES) |
Code Review — PR #463This PR cleanly extracts the inline schedule logic into a dedicated Python module and adds a meaningful test suite. The overall direction is good. Below are the issues I found, ordered by severity. Bugs / Regressions1. # new
REPOSITORIES_PER_DAY: ${{ github.event.inputs.repositories_per_day || env.REPOS_PER_DAY }}
# old
REPOSITORIES_PER_DAY: ${{ github.event.inputs.repositories_per_day || '10' }}If REPOSITORIES_PER_DAY: ${{ github.event.inputs.repositories_per_day || env.REPOS_PER_DAY || '10' }}2. Unquoted git add ${{ env.SCHEDULE_FILE }} # ← missing quotesThe surrounding Security Concerns3. STAGGER_MINUTES: ${{ needs.calculate-schedule.outputs.stagger_minutes || '5' }}
…
sleep $((STAGGER_MINUTES * 60))
# guard in execute_schedule.sh
[[ "$STAGGER_MINUTES" =~ ^[0-9]+$ ]] || STAGGER_MINUTES=5Code Quality4. repos_output.write_text("\n".join(repositories) + ("\n" if repositories else ""))
repos_output.write_text("".join(repo + "\n" for repo in repositories))This is a nit — not a functional bug. 5.
"has_schedule": "true" if schedule.get("total_repositories", 0) > 0 else "false",Test CoverageThe five unit tests cover the happy paths well. A few edge cases worth adding:
Minor
Overall: the refactor improves correctness and testability. Fix items 1 and 2 before merging — the |
|
Warning Review limit reached
More reviews will be available in 53 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ 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 |
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR introduces a reusable Python-based staggered schedule planner, but contains critical regressions: it fails to detect when the repository list or configuration changes, causing new repositories to be missed and schedule drift to go unnoticed.
📄 Documentation Diagram
This diagram documents the refactored staggered walkthrough scheduling workflow.
sequenceDiagram
participant W as Workflow
participant RL as Repos List
participant SS as Schedule Script
participant GR as Git Repo
W->>RL: fetch active repos
RL-->>W: repositories.json
W->>SS: plan schedule (force, repos_per_day)
SS->>SS: decide reuse or regenerate
note over SS: PR #35;463: New script reuse active schedule if pending batches exist
SS-->>W: schedule.json
W->>GR: save & commit schedule
W->>SS: check today's due
SS-->>W: due repos (repos_today.txt)
W->>GR: trigger workflow per repo<br/>with stagger & concurrency
W->>SS: mark date complete
SS-->>W: completed schedule
🌟 Strengths
- Solid test coverage for core logic, ensuring reliability.
- Modular design with clear separation of concerns.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | src/.../staggered_walkthrough_schedule.py | Bug | Misses detection of repository or config changes | method:plan_schedule |
| P1 | src/.../staggered_walkthrough_schedule.py | Bug | Ignores change in repos_per_day on reuse | method:command_plan |
| P2 | tests/.../test_staggered_walkthrough_schedule.py | Testing | No test for repo list change regeneration | |
| P2 | .github/workflows/staggered-scheduling.yml | Documentation | Variable names not updated in docs | path:docs/guides/... |
| P2 | src/.../staggered_walkthrough_schedule.py | Testing | Missing edge case coverage |
🔍 Notable Themes
- Config drift blindness: Both P1 findings stem from the same root cause: the schedule reuse code does not compare the current configuration (repos list, repos_per_day) against the stored schedule. This systemic risk could cause extended scheduling gaps. Consider adding a fingerprinting approach to detect drifts automatically.
📈 Risk Diagram
This diagram illustrates the stale schedule reuse risk and its potential impact.
sequenceDiagram
participant U as User
participant W as Workflow
participant SS as Schedule Script
participant R as Repository List
U->>W: trigger (new repos / changed config)
W->>R: fetch current repos (updated)
R-->>W: repositories.json (changed)
W->>SS: plan schedule (new repos, new repos_per_day)
SS->>SS: check existing schedule (no config comparison)
SS-->>W: return old schedule (stale)
W->>W: execute old schedule (misses new repos)
note over SS: R1(P1): Config drift not detected<br/>R2(P1): repos_per_day change ignored
⚠️ **Unanchored Suggestions (Manual Review Recommended)**
The following suggestions could not be precisely anchored to a specific line in the diff. This can happen if the code is outside the changed lines, has been significantly refactored, or if the suggestion is a general observation. Please review them carefully in the context of the full file.
📁 File: src/automation/scripts/staggered_walkthrough_schedule.py
The reuse logic in plan_schedule does not validate that the existing schedule’s repos_per_day value matches the current repos_per_day parameter. If a workflow user changes the REPOSITORIES_PER_DAY input (or the default env variable), the reused schedule will still use the old number of repositories per day, leading to an inconsistent schedule. This is a deterministic bug path when the config changes while pending batches exist.
Suggestion:
if existing_schedule and pending_schedule_dates(existing_schedule):
stored_repos_per_day = existing_schedule.get("repos_per_day")
if stored_repos_per_day is not None and stored_repos_per_day != repos_per_day:
new_schedule = generate_schedule(...)
return new_schedule, True, "repos_per_day changed"
return existing_schedule, False, "active schedule has pending batches"Related Code:
def plan_schedule(
repositories: list[dict[str, Any]],
existing_schedule: dict[str, Any] | None,
repos_per_day: int,
...
) -> tuple[dict[str, Any], bool, str]:
if existing_schedule and pending_schedule_dates(existing_schedule):
return existing_schedule, False, "active schedule has pending batches"💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| return sorted(pending_dates) | ||
|
|
||
|
|
||
| def plan_schedule( |
There was a problem hiding this comment.
P1 | Confidence: High
The plan_schedule function reuses an existing schedule when it has pending batches, without verifying that the set of repositories or repos_per_day configuration has changed. In the old workflow, the schedule was always regenerated from the current repository list. This change introduces a regression: if a new repository is added or an existing one is removed, the reused schedule will not reflect the updated list. Similarly, if a user changes the REPOSITORIES_PER_DAY input, the old schedule (with a different repos_per_day) will be incorrectly reused. This could cause new repositories to be missed from scheduling for an extended period (until all pending batches are completed), which is a real data-flow risk given that repository discovery runs in the same workflow before plan_schedule is called.
Code Suggestion:
def _schedule_fingerprint(repositories: list[dict], repos_per_day: int, stagger_minutes: int) -> str:
import hashlib
key = (json.dumps([r.get("full_name") for r in repositories], sort_keys=True),
repos_per_day, stagger_minutes)
return hashlib.sha256(repr(key).encode()).hexdigest()
def plan_schedule(...) -> tuple[dict[str, Any], bool, str]:
if force:
new_schedule, _, _ = generate_schedule(...)
return new_schedule, True, "forced"
if existing_schedule:
stale = existing_schedule.get("_fingerprint") != _schedule_fingerprint(repositories, repos_per_day, stagger_minutes)
if stale or not pending_schedule_dates(existing_schedule):
new_schedule, _, _ = generate_schedule(...)
new_schedule["_fingerprint"] = _schedule_fingerprint(repositories, repos_per_day, stagger_minutes)
return new_schedule, True, "regenerated due to config change" if stale else "no pending batches"
return existing_schedule, False, "active schedule has pending batches"
new_schedule, _, _ = generate_schedule(...)
new_schedule["_fingerprint"] = _schedule_fingerprint(repositories, repos_per_day, stagger_minutes)
return new_schedule, True, "schedule file missing"Evidence: method:plan_schedule
| assert schedule["schedule"]["2026-06-02"]["repositories"] == ["organvm/large-app"] | ||
|
|
||
|
|
||
| @pytest.mark.unit |
There was a problem hiding this comment.
P2 | Confidence: High
The test suite does not include any scenario where the repository list changes between schedule generations. This means the bug described in the first finding (missing detection of repo list changes) will not be caught by the current tests. Adding a test where the repositories argument contains different repos than the existing schedule would demonstrate the gap.
Code Suggestion:
def test_plan_schedule_regenerates_when_repositories_change(repositories):
existing = generate_schedule(repositories, repos_per_day=2, start_date=date(2026, 6, 1))
changed_repos = repositories + [{"name": "new-app", "full_name": "organvm/new-app", "size": 10}]
planned, regenerated, _ = plan_schedule(changed_repos, existing, repos_per_day=2, today=date(2026, 6, 2))
assert regenerated is True
assert "organvm/new-app" in [repo for day in planned["schedule"].values() for repo in day["repositories"]]| def pending_schedule_dates(schedule: dict[str, Any]) -> list[date]: | ||
| """Return pending schedule dates in ascending order.""" | ||
| entries = schedule.get("schedule") or {} | ||
| if not isinstance(entries, dict): |
There was a problem hiding this comment.
P2 | Confidence: High
The test file covers the main happy paths for generate_schedule, plan_schedule, select_due_batch, and mark_completed, but it does not test edge cases such as: empty repository list, select_due_batch when today exactly matches a scheduled date, select_due_batch when there are no pending dates, mark_completed with a non-existent date, and plan_schedule with force=True. These gaps reduce confidence in robustness.
| echo "repo_count=$REPO_COUNT" >> $GITHUB_OUTPUT | ||
|
|
||
| - name: Generate staggered schedule | ||
| - name: Plan staggered schedule |
There was a problem hiding this comment.
P2 | Confidence: Medium
The documentation guide docs/guides/AUTOMATION_GUIDE.md mentions configuration options including repos_per_day, but the new workflow uses REPOSITORIES_PER_DAY as the environment variable name and REPOS_PER_DAY as the fallback env. The guide is not updated to reflect the new variable name or the new --force input. This is a maintainability concern.
|
Backlog engagement 2026-07-19 — disposition: superseded by current main. Verified live state: CONFLICTING/DIRTY with 12 failing checks. Current main already contains |
1 participant
Autonomous limen dispatch of task
RESOLVE-organvm-i-theoria-.github-458.Resolve blocked PR #458 ('[limen CIFIX-organvm-i-theoria--github] Fix pre-existing CI '), state=BLOCKED. Branch=limen/cifix-organvm-i-theoria--github-64ce, base=main. In the worktree:
git fetch origin limen/cifix-organvm-i-theoria--github-64ce main; git checkout -B limen/cifix-organvm-i-theoria--github-64ce origin/limen/cifix-organvm-i-theoria--github-64ce; git rebase origin/mainthen address the failing checks / unresolved review threads; run the build/tests;git push --force-with-lease origin limen/cifix-organvm-i-theoria--github-64ce. If the branch is unrecoverable, instead REBUILD the same feature cleanly off origin/main as a fresh PR. Goal: make it mergeable.Produced in an isolated worktree off origin — review before merge.
Summary by Sourcery
Introduce a reusable Python-based planner for staggered walkthrough scheduling and integrate it into the GitHub Actions workflow to make scheduling, execution, and completion tracking more robust and flexible.
Enhancements:
Tests: