26: temporal and scoped authorization facts - #44
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughAuthorization decisions now support scope, temporal validity, and recurring schedules. Fact validation and denial reasons are expanded, evaluation options accept scope and timestamps, and tests cover matching, precedence, timezone, overnight, and boundary behavior. ChangesAuthorization evaluation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Review Post-Mortem: Critic FindingsAfter the critic+defender review, here's what was found and what was done: ✅ Fixed
⏳ Deferred (documented, not fixed)
📋 Next steps
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/src/index.ts (1)
366-382: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a strict ISO 8601 parser here.
new Date()accepts engine-specific non-ISO strings, so this check can let invalid values through and makeisTemporallyActive()behave differently across runtimes. A stricter parse/validation step would keep these fact timestamps deterministic.🤖 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 `@packages/core/src/index.ts` around lines 366 - 382, Replace the permissive new Date() validation for fact.startsAt and fact.expiresAt in the contract-validation block with the project’s strict ISO 8601 parsing or validation utility. Preserve the existing TypeError messages and ensure both optional timestamps reject non-ISO values deterministically before isTemporallyActive() can use them.Source: Path instructions
🤖 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/engineering-workflow.md`:
- Around line 193-205: Reconcile the sequencing between Step 4 and the
`implement` workflow described around line 142: ensure adversarial review
findings are addressed before the `code-review` skill runs and before changes
are committed. Update the relevant workflow text so the order is explicit and no
step implies reviewing or committing prematurely.
In `@packages/core/src/index.ts`:
- Around line 567-603: Update the weekly and date-window checks in the schedule
evaluation flow to use a same-day range check that excludes the overnight
early-morning portion on the listed day, while preserving isOvernightNextActive
for the next-day extension. Add coverage for a Wednesday 22:00–02:00 window
evaluated Wednesday at 01:00, asserting the result is outside-schedule.
---
Nitpick comments:
In `@packages/core/src/index.ts`:
- Around line 366-382: Replace the permissive new Date() validation for
fact.startsAt and fact.expiresAt in the contract-validation block with the
project’s strict ISO 8601 parsing or validation utility. Preserve the existing
TypeError messages and ensure both optional timestamps reject non-ISO values
deterministically before isTemporallyActive() can use them.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 45c7b675-4000-42a4-9c1a-a4dc07717534
📒 Files selected for processing (4)
.gitignoredocs/engineering-workflow.mdpackages/core/__tests__/decision.test.tspackages/core/src/index.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Both CodeRabbit findings from the previous review have been addressed in commit d710224: 1. Overnight early-morning over-match 🟠
2. Workflow ordering 🟠
All 89 tests pass, typecheck + build pass. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/engineering-workflow.md`:
- Around line 142-144: Update the upstream implement contract in the ask-matt
skill so /implement no longer runs or commits after the final code-review before
adversarial fixes; align its sequencing with the workflow’s deferred step 5.5
review, and revise the “already run by implement” wording near the related
documentation section to distinguish preliminary versus final reviews.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eb658fca-bb3e-42e2-a1a0-ccfd1efc32a7
📒 Files selected for processing (3)
docs/engineering-workflow.mdpackages/core/__tests__/decision.test.tspackages/core/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/tests/decision.test.ts
- packages/core/src/index.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
Adversarial Review ResultsCritic vs Defender — Full Breakdown
What the defender said overall
Final judgment
Deferred item
|
|
Deferred item tracked in issue #45: DST correctness for overnight schedule windows. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Debate Meta-Review — All 18 Findings Accounted For✅ Fixed in this PR (13 items)
📝 Deferred to new issues (5 items)
Nothing dismissed. Every finding has a concrete path forward. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/engineering-workflow.md`:
- Around line 207-209: Update the workflow table’s “Any production code” row and
the stale wording near the implementation step so the code-review routing is
consistent: identify it as the final review at step 5.5 after debate and
test-audit fixes, or explicitly label the earlier step 4.5 review as
preliminary. Preserve the separate debate and test-audit routing.
- Around line 511-527: Reconcile the merge-authority policy across
docs/engineering-workflow.md by choosing either agent-controlled merging after
explicit approval or human-only merging. Update the “Merge Rule: Agent merges
only on explicit instruction” section and every other merge-related instruction
consistently, removing contradictory requirements while preserving the selected
policy’s conditions and safeguards.
- Around line 567-580: Update all three inline-comment fetch commands to use gh
api --paginate. In docs/engineering-workflow.md lines 567-580, also remove the
.body[:100] jq truncation so the complete comment body is displayed; apply
pagination only to the corresponding commands in .agents/skills/guard/SKILL.md
lines 55-58 and .agents/wisdom/adversarial-review.md lines 55-58.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e84d93c-5105-4ef2-a8fd-018992bd218a
📒 Files selected for processing (11)
.agents/skills/capture-wisdom/SKILL.md.agents/skills/debate/SKILL.md.agents/skills/feature-implement/SKILL.md.agents/skills/guard/SKILL.md.agents/skills/implement/SKILL.md.agents/skills/test-audit/SKILL.md.agents/wisdom/adversarial-review.md.agents/wisdom/testing.mddocs/engineering-workflow.mdpackages/core/__tests__/decision.test.tspackages/core/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/tests/decision.test.ts
- packages/core/src/index.ts
Cherry-picked from feature/26-temporal-scoped-facts branch. Only packages/core/ files — no workflow changes.
f7eeee6 to
a5ceb1f
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Replaced permissive new Date() fallback with logical date validation: - Parse year/month/day from regex capture groups - Validate month range (01-12) - Validate day range (1-max for month, with leap year support) - Rejects logically invalid dates like Feb 30, month 13 - Accepts Feb 29 in leap years, rejects in non-leap years - No new Date() parsing involved — regex + arithmetic only
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/__tests__/decision.test.ts (1)
547-603: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate source mock definitions and ensure time-independent assertions.
You can reduce boilerplate in the rejection tests by reusing the
sourceWithhelper, just as you did in the valid leap year test. Additionally, consider passing an explicitatoption for the valid leap year test so the.toBe(true)assertion remains completely deterministic and independent of the system clock.♻️ Proposed refactor
it("throws when a source returns a fact with JavaScript-parseable but non-ISO startsAt", async () => { const mizan = createMizan(); - mizan.registerSource("bad", { - async resolve() { - return { status: "facts", facts: [{ permission: "x", effect: "grant", startsAt: "January 1, 2026" }] }; - }, - }); + mizan.registerSource("bad", sourceWith({ permission: "x", effect: "grant", startsAt: "January 1, 2026" })); const auth = mizan.forPrincipal("user-1"); await expect(auth.can("x")).rejects.toThrow(/contract violation/i); }); it("throws when a source returns a fact with American-format date as startsAt", async () => { const mizan = createMizan(); - mizan.registerSource("bad", { - async resolve() { - return { status: "facts", facts: [{ permission: "x", effect: "grant", startsAt: "12/25/2024" }] }; - }, - }); + mizan.registerSource("bad", sourceWith({ permission: "x", effect: "grant", startsAt: "12/25/2024" })); const auth = mizan.forPrincipal("user-1"); await expect(auth.can("x")).rejects.toThrow(/contract violation/i); }); it("throws when a source returns a fact with logically invalid ISO date (Feb 30)", async () => { const mizan = createMizan(); - mizan.registerSource("bad", { - async resolve() { - return { status: "facts", facts: [{ permission: "x", effect: "grant", startsAt: "2024-02-30T00:00:00Z" }] }; - }, - }); + mizan.registerSource("bad", sourceWith({ permission: "x", effect: "grant", startsAt: "2024-02-30T00:00:00Z" })); const auth = mizan.forPrincipal("user-1"); await expect(auth.can("x")).rejects.toThrow(/contract violation/i); }); it("accepts valid leap year date (Feb 29, 2024)", async () => { const mizan = createMizan(); mizan.registerSource("mem", sourceWith({ permission: "x", effect: "grant", startsAt: "2024-02-29T00:00:00Z" })); const auth = mizan.forPrincipal("user-1"); // Should not throw — Feb 29, 2024 is a valid leap year date - await expect(auth.can("x")).resolves.toBe(true); + await expect(auth.can("x", { at: new Date("2024-03-01T00:00:00Z") })).resolves.toBe(true); }); it("rejects Feb 29 in non-leap year (2023)", async () => { const mizan = createMizan(); - mizan.registerSource("bad", { - async resolve() { - return { status: "facts", facts: [{ permission: "x", effect: "grant", startsAt: "2023-02-29T00:00:00Z" }] }; - }, - }); + mizan.registerSource("bad", sourceWith({ permission: "x", effect: "grant", startsAt: "2023-02-29T00:00:00Z" })); const auth = mizan.forPrincipal("user-1"); await expect(auth.can("x")).rejects.toThrow(/contract violation/i); });🤖 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 `@packages/core/__tests__/decision.test.ts` around lines 547 - 603, Refactor the invalid-date rejection tests around sourceWith to reuse the existing helper instead of repeating inline bad source definitions. In the valid leap-year test, pass an explicit at option to auth.can so the true assertion is deterministic and does not depend on the current system time.packages/core/src/index.ts (1)
500-560: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
isStrictISODatedoesn't bound hour/minute/second components.
ISO_DATE_REmatches\d{2}for hour/minute/second without range checks, so a value like"2024-01-01T99:99:99Z"passesisStrictISODatedespite being logically invalid. The function validates year/month/day ranges (including leap years) but not time-of-day ranges, even though it gatesstartsAt/expiresAtfull timestamps used later for temporal window evaluation.🐛 Proposed fix
const maxDay = DAYS_IN_MONTH[month - 1]! + (month === 2 && isLeapYear(year) ? 1 : 0); if (day > maxDay) { return false; } + // Validate time components when present (hour 00-23, minute/second 00-59). + const timeMatch = /T(\d{2}):(\d{2}):(\d{2})/.exec(value); + if (timeMatch) { + const hour = Number.parseInt(timeMatch[1]!, 10); + const minute = Number.parseInt(timeMatch[2]!, 10); + const second = Number.parseInt(timeMatch[3]!, 10); + if (hour > 23 || minute > 59 || second > 59) { + return false; + } + } return true; }🤖 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 `@packages/core/src/index.ts` around lines 500 - 560, Update ISO_DATE_RE and isStrictISODate to validate time-of-day components as well as date components, rejecting hours above 23 and minutes or seconds above 59 for datetime inputs such as “2024-01-01T99:99:99Z”. Preserve acceptance of valid dates, optional fractional seconds, and supported timezone formats.
🤖 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.
Nitpick comments:
In `@packages/core/__tests__/decision.test.ts`:
- Around line 547-603: Refactor the invalid-date rejection tests around
sourceWith to reuse the existing helper instead of repeating inline bad source
definitions. In the valid leap-year test, pass an explicit at option to auth.can
so the true assertion is deterministic and does not depend on the current system
time.
In `@packages/core/src/index.ts`:
- Around line 500-560: Update ISO_DATE_RE and isStrictISODate to validate
time-of-day components as well as date components, rejecting hours above 23 and
minutes or seconds above 59 for datetime inputs such as “2024-01-01T99:99:99Z”.
Preserve acceptance of valid dates, optional fractional seconds, and supported
timezone formats.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f39ff8cb-b918-4d37-b6af-f932a4865f00
📒 Files selected for processing (2)
packages/core/__tests__/decision.test.tspackages/core/src/index.ts
1 participant
What
Facts now support scope, temporal windows (startsAt/expiresAt), and recurring schedules (weekly windows, date-specific windows, overnight windows, IANA timezones).
Key changes
"not-yet-active"toDenyReason.PrincipalEvaluator.can()anddecide()accept optionalEvaluateOptions(scope, at), backward compatible.Tests
34 new tests covering:
All 77 tests pass, typecheck passes, build passes.
Refs #26
Summary by CodeRabbit
New Features
not-yet-activeandoutside-schedule, with clearer precedence between out-of-scope, inactive, and schedule-mismatched results.can()anddecide()accept options to control scope and evaluation time.Bug Fixes
Tests