feat: enforce R-FC-013 (open item in past cycle -> current cycle) hourly - #57
Merged
Merged
Conversation
Adds a new mechanical rule alongside R-FC-012: any open issue or PR whose Cycle points at an already-ended iteration gets moved to the current cycle, so stale cycle assignments don't hide in-flight work from cycle planning. Reuses R-FC-012's iteration-fetch and mutation-log-guard pattern so a human who deliberately moves an item back to a past cycle won't get overridden. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015WuAyMAjh1sno5spL7R3fV
GitHub's ProjectV2IterationField.configuration.iterations only returns current and future iterations -- completed ones live under the separate completedIterations field. A live dry-run against the real board showed every past-cycle item being classified as "not a past iteration (future or unknown)" because we never fetched that history. Combine both lists in _fetch_iterations so R-FC-013 can actually see past cycles; R-FC-012 is unaffected since it only ever needs the iteration containing today. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015WuAyMAjh1sno5spL7R3fV
Contributor
Author
R-FC-013 dry-run against the live boardRan Found a real bug in the process: the first dry-run showed 0 items ever matching "past cycle" -- GitHub's GraphQL schema splits current/future iterations from completed ones ( Result after the fix: current cycle is All 179 mutations R-FC-013 would apply (dry-run, nothing changed on the board)R-FC-013 (cycle) — applied=179, skipped=3
|
There was a problem hiding this comment.
Pull request overview
Adds hourly R-FC-013 enforcement to move open items from completed cycles into the current cycle.
Changes:
- Adds and registers
PastCycleRulewith mutation-log safeguards. - Refactors cycle lookup and adds tests.
- Updates mechanical-rule and board documentation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Review summary |
|---|---|
foc-mechanical-rules/tests/test_past_cycle_rule.py |
Adds coverage for R-FC-013 behavior. |
foc-mechanical-rules/README.md |
Documents the new rule and safeguards. |
foc-mechanical-rules/foc_mechanical_rules/rules/cycle.py |
Critical (4): completed iterations are not queried. Critical (3): draft notes are included despite the issue/PR scope. Moderate (4): mutations add per-item reads. Moderate (2): mutation logs use a stale cycle value instead of the API-observed value. |
foc-mechanical-rules/foc_mechanical_rules/registry.py |
Critical (1): overlapping workflow runs can race because no concurrency or durable locking is configured. |
foc-board-rules/future-ideas.md |
Updates the implemented-rule inventory. |
foc-board-rules/field-completeness.md |
Defines canonical R-FC-013 behavior. |
Suppressed comments (1)
foc-mechanical-rules/tests/test_past_cycle_rule.py:32
- The fixture puts both ended cycles under
configuration.iterations, but the actual GraphQL contract used elsewhere in this repo returns ended cycles underconfiguration.completedIterations(github-projects-client/github_projects_client/fields.py:31-42). That lets these tests pass while the implementation ignores completed cycles—the exact production failure. Put the two past entries incompletedIterationsand leave only active/future entries initerationsso this test exercises the real response shape.
# GitHub's GraphQL schema splits current/future iterations
# from completed ones -- ``iterations`` alone never
# includes a truly past cycle. Mirror that split here so
# a regression back to reading only ``iterations`` fails
# this test instead of passing by accident.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Serialize the hourly workflow (concurrency group, cancel-in-progress: false) so overlapping runs can't race on the shared mutation-log cache. - Skip draft notes: has:cycle matches any board item, including notes with no linked repo issue/PR, which had no Repository/Id to mutate. - Pass the item's node ID (already returned by list_items) instead of "owner/repo#number" to set_field_value, skipping its internal per-item get_item lookup and recording the API-observed old_value instead of the value seen at selection time (which race-guard PRs correctly distrust). Verified against the live board (dry-run): same 179 applied / 3 skipped as before this commit, confirming the refactor didn't change outcomes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015WuAyMAjh1sno5spL7R3fV
Contributor
Author
|
Addressed all 5 Copilot review findings (concurrency race, draft-note guard, per-item read via node ID, stale old_value, and the already-fixed completedIterations gap) — see 3398cb3 and inline replies. Re-ran the dry-run against the live board after the fixes: same 179 applied / 3 skipped, confirming the refactor didn't change outcomes. |
Validating one new rule against the live board (e.g. this PR's R-FC-013) meant reading its section out of a combined log for every registered rule. --rule R-FC-013 (repeatable) filters to just the named rule(s); an unknown id exits with a list of known ones instead of silently running everything. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015WuAyMAjh1sno5spL7R3fV
…-012 - Add a README "API call pattern per rule" table (select() cost, per-run memoized reads, per-item reads/writes) so the call shape of each rule is reasoned about wholistically instead of read out of the source. Points out R-PR-001's per-PR REST reads as a known, currently-acceptable gap with no batched equivalent. - Apply R-FC-013's node-ID mutation fix to R-FC-012 (CycleRule) too, for the same reason: skips set_field_value_bulk's per-item get_item lookup. - Log batch mutations (github_projects_client already batches GraphQL mutations 25-at-a-time, but every rule always calls it with a 1-item list) as a future-ideas.md follow-up -- it requires restructuring the shared Rule.run()/apply_one contract, so it's out of scope here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015WuAyMAjh1sno5spL7R3fV
2 of 3 tasks
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
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.
Summary
PastCycleRuleinfoc-mechanical-rules, alongside the existing R-FC-012 (CycleRule) — shares the iteration-lookup logic (refactored intoget_current_and_past_cycle_titles) and the same mutation-log-guard pattern, so a human who deliberately moves an item back to a past cycle won't get overridden.registry.py, so it runs automatically in the existing hourly GitHub Actions workflow — no workflow changes needed.has:cycle+Cyclefield), so no per-item reads are added; the only per-item API calls are writes, and only for items actually being moved.Test plan
uv run pytest -m "not integration"— all 32 tests pass, including 8 new tests forPastCycleRule(move-to-current, already-current skip, future-cycle skip, human-reversion flag, non-blocking prior history for a different cycle, no-active-cycle error, dry-run,isinstance(Rule))uv run foc-mechanical-rules --dry-run) once merged/deployed — noGITHUB_TOKENwas available in this session to verify against real board dataCo-Authored-By: Claude Sonnet 5 noreply@anthropic.com
https://claude.ai/code/session_015WuAyMAjh1sno5spL7R3fV