chore: refine HITL specs and rename change#21
Conversation
Add OpenSpec framework structure with: - Core spec (20 requirements) for implemented features - 4 change proposals for planned features: - add-human-in-loop: ctx.human() API, waiting_human status - add-streaming-v2: step.stream(), event persistence - add-job-versioning: job_hash for run-job binding - add-postgresql: PostgreSQL dialect support Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR introduces a comprehensive specification and documentation framework for the Durably project via OpenSpec, including core architecture specifications, project conventions, and structured proposals for four major features: Job Versioning, PostgreSQL Support, Streaming v2, and Human-in-the-Loop workflows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@openspec/changes/add-human-in-loop/specs/core/spec.md`:
- Around line 94-105: Update the GET /runs scenarios to require explicit
authorization before returning wait_token: only return wait_token when the query
param includeToken=true AND the caller has explicit permission (e.g., a new
required scope/claim such as "runs:read:token" or an equivalent authz check).
Modify the Scenario: GET /runs with includeToken to state that the request must
include includeToken=true and valid authorization that grants token access, and
add a note to Scenario: GET /runs without includeToken that no token is returned
unless both includeToken=true and the required authz is present.
In `@openspec/changes/add-streaming-v2/proposal.md`:
- Around line 18-22: The Impact section mixes Japanese with English; update the
listed entries to use consistent English wording (e.g., replace "追加" with
"added", "テーブル操作追加" with "added table operations", "拡張" with "extended",
"テーブル定義" with "table definition", "マイグレーション" with "migration") for the lines
referencing packages/durably/src/context.ts (step.stream()),
packages/durably/src/storage.ts (events table operations),
packages/durably/src/durably.ts (subscribe()), packages/durably/src/schema.ts
(events table definition), and packages/durably/src/migrations.ts (events table
migration) so the entire Impact section is fully English and consistent.
🧹 Nitpick comments (10)
openspec/changes/add-postgresql/proposal.md (1)
9-21: Clarify dialect selection/config path.
Please specify how PostgreSQL is selected at runtime (e.g., configuration flag, env, or explicit factory likecreateDurably({ dialect })) and whether PG support is optional or required for certain deployments. This will prevent ambiguity in implementation tasks.openspec/changes/add-job-versioning/tasks.md (1)
21-33: Add a task for API/contract docs + error mapping.
Consider adding a checklist item to update public docs/API references forjob_version_mismatch,allowIncompatible, and any HTTP status mapping (e.g., 412) so implementation and docs stay aligned.openspec/changes/add-job-versioning/proposal.md (1)
18-22: Standardize language for consistency.
This block mixes English and Japanese terms. Consider standardizing to a single language for consistency across OpenSpec docs.openspec/project.md (2)
105-117: Confirm repository layout matchesdocs/andopenspec/.
If specs live underopenspec/, consider updating this tree to avoid confusion for contributors.
89-97: Consider adding the top‑levelawaitconstraint.
Given the ESM-only constraint, it may help to note the top‑levelawaitrequirement for async initialization (e.g.,await durably.migrate()) to avoid incorrect patterns. Based on learnings, ...openspec/changes/add-job-versioning/specs/core/spec.md (1)
7-12: Define canonicalization/normalization for stability.
To guarantee order-independence, consider specifying the exact canonicalization strategy (e.g., stable JSON serialization, sorted keys, normalized schema representation).openspec/AGENTS.md (1)
26-31: Optional: consolidate repeated example phrases.The adjacent “I want to create a spec proposal / spec” bullets are redundant; consider merging to keep the trigger list tighter.
openspec/changes/add-streaming-v2/specs/core/spec.md (2)
31-58: Clarify sequence scope and replay ordering guarantees.Please specify whether
sequenceis per-run or global, and whether ordering is strictly monotonic across all persisted event types. Also clarify the replay boundary (inclusive/exclusive) and ordering between replayed events and the first live event to avoid divergent implementations.
20-25: Explicitly note that stream events are not replayed.Given
streamevents are memory-only, please state in the replay scenario that no prior stream events will be re-emitted on resume/replay, so consumers don’t expect token replay.Also applies to: 34-35
openspec/specs/core/spec.md (1)
73-77: Consider retention policy for perpetual idempotency keys.“No expiration” can create unbounded storage growth and retention risk. Consider documenting an optional retention/cleanup policy or a maintenance job to prune old idempotency records if product requirements allow.
| #### Scenario: GET /runs with includeToken | ||
|
|
||
| - **GIVEN** Run is in `waiting_human` state | ||
| - **WHEN** `GET /runs?status=waiting_human&includeToken=true` is called | ||
| - **THEN** response includes `wait_token` | ||
|
|
||
| #### Scenario: GET /runs without includeToken | ||
|
|
||
| - **GIVEN** Run is in `waiting_human` state | ||
| - **WHEN** `GET /runs?status=waiting_human` is called (without includeToken) | ||
| - **THEN** response does NOT include `wait_token` | ||
|
|
There was a problem hiding this comment.
Require explicit authz for includeToken to avoid token leakage.
wait_token is a bearer credential; the spec should mandate authorization/least-privilege and explicit opt‑in checks before returning it to clients.
🤖 Prompt for AI Agents
In `@openspec/changes/add-human-in-loop/specs/core/spec.md` around lines 94 - 105,
Update the GET /runs scenarios to require explicit authorization before
returning wait_token: only return wait_token when the query param
includeToken=true AND the caller has explicit permission (e.g., a new required
scope/claim such as "runs:read:token" or an equivalent authz check). Modify the
Scenario: GET /runs with includeToken to state that the request must include
includeToken=true and valid authorization that grants token access, and add a
note to Scenario: GET /runs without includeToken that no token is returned
unless both includeToken=true and the required authz is present.
| - `packages/durably/src/context.ts` - `step.stream()` 追加 | ||
| - `packages/durably/src/storage.ts` - `events` テーブル操作追加 | ||
| - `packages/durably/src/durably.ts` - `subscribe()` 拡張 | ||
| - `packages/durably/src/schema.ts` - `events` テーブル定義 | ||
| - `packages/durably/src/migrations.ts` - events テーブルマイグレーション |
There was a problem hiding this comment.
Normalize language in Impact section.
This section mixes English with Japanese (“追加”, “テーブル操作追加”, etc.). Consider keeping it fully English for consistency with the rest of the doc.
🤖 Prompt for AI Agents
In `@openspec/changes/add-streaming-v2/proposal.md` around lines 18 - 22, The
Impact section mixes Japanese with English; update the listed entries to use
consistent English wording (e.g., replace "追加" with "added", "テーブル操作追加" with
"added table operations", "拡張" with "extended", "テーブル定義" with "table
definition", "マイグレーション" with "migration") for the lines referencing
packages/durably/src/context.ts (step.stream()), packages/durably/src/storage.ts
(events table operations), packages/durably/src/durably.ts (subscribe()),
packages/durably/src/schema.ts (events table definition), and
packages/durably/src/migrations.ts (events table migration) so the entire Impact
section is fully English and consistent.
Keep spec.md in English for OpenSpec validation and OSS accessibility, while using Japanese for proposal.md and tasks.md for faster authoring. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.claude/commands/openspec/apply.md:
- Line 8: Replace the section titles that use bold emphasis (e.g.,
"**Guardrails**" and other occurrences like "**Reference**") with proper
Markdown heading syntax (for example, change "**Guardrails**" to "##
Guardrails"); update all similar emphasis-as-heading instances in the file
(including the other reported occurrence) so they use the appropriate heading
level and run markdownlint to verify no emphasis-as-heading warnings remain.
In `@docs/spec-human-in-loop.md`:
- Around line 59-63: Add the language identifier "text" to the fenced code block
containing the ASCII state transition diagram (the block that starts with
"pending -> running -> waiting_human -> running -> completed" and the
accompanying lines with "failed"), i.e., change the opening triple backticks to
```text so the diagram is treated as plain text.
♻️ Duplicate comments (1)
openspec/changes/add-streaming-v2/proposal.md (1)
18-22: Normalize language in Impact section (already noted).
🧹 Nitpick comments (4)
.claude/commands/openspec/archive.md (1)
8-13: Use proper headings instead of bold labels.Markdownlint flags these as emphasis-as-heading; switch to
##/###to keep lint clean.♻️ Proposed fix
-**Guardrails** +## Guardrails ... -**Steps** +## Steps ... -**Reference** +## ReferenceAlso applies to: 24-24
docs/spec-job-versioning.md (1)
37-39: Convert the bold line to a heading.Markdownlint MD036 will flag this; prefer a heading for consistency.
♻️ Proposed fix
-**HITL との整合（Phase 2）** +### HITL との整合（Phase 2）docs/spec-human-in-loop.md (1)
1-492: Well-structured HITL specification with comprehensive coverage.This specification thoroughly documents the Human-in-the-Loop feature including:
- Clear API surface (ctx.human, durably.resume)
- Complete data model with security considerations
- HTTP endpoint specifications with error handling
- Implementation guidance with SQL migrations and transaction boundaries
- UX considerations and usage examples
The phased rollout approach (Phase 1: HITL standalone, Phase 2: Job Versioning integration) demonstrates good architectural planning.
Optional: Consider using markdown headings for subsections
Several subsections use bold text (e.g., lines 50, 72, 80, 148, 167, 177, 224, 282, 289, 294, 308, 342, 348, 401) instead of proper markdown headings. While this creates a more compact document, using
####or#####headings would improve document structure, enable better navigation, and resolve the markdownlint warnings.This is purely a documentation quality suggestion and not blocking.
docs/spec-react.md (1)
530-599: Excellent HITL API design with good security boundaries.The
useHumanInboxhook provides a clean, developer-friendly interface for Human-in-the-Loop workflows:
- Single-hook approach simplifies the API surface (no separate
useWaitingRuns/useResume)- Convenience methods (
approve,reject,edit) automatically populate thedecisionfield- Security-conscious:
wait_tokenis kept internal to the hook, not exposed in theWaitingRuntype- Consistent with endpoints defined in
docs/spec-human-in-loop.mdThe separation between server-connected mode (
@coji/durably-react/client) and browser-only mode (@coji/durably-react) with the same API surface is elegant.Optional: Subsection heading formatting
Lines 572 and 596 use bold text for subsection headers ("戻り値" and "内部動作"). Consider using proper markdown headings (
#####) for better document structure, though the current formatting is acceptable.
| tags: [openspec, apply] | ||
| --- | ||
| <!-- OPENSPEC:START --> | ||
| **Guardrails** |
There was a problem hiding this comment.
Use headings instead of bold text for section titles.
Markdownlint flags emphasis-as-heading here; switch to actual heading syntax (e.g., ## Guardrails, ## Reference) for consistency.
Also applies to: 21-21
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
8-8: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🤖 Prompt for AI Agents
In @.claude/commands/openspec/apply.md at line 8, Replace the section titles
that use bold emphasis (e.g., "**Guardrails**" and other occurrences like
"**Reference**") with proper Markdown heading syntax (for example, change
"**Guardrails**" to "## Guardrails"); update all similar emphasis-as-heading
instances in the file (including the other reported occurrence) so they use the
appropriate heading level and run markdownlint to verify no emphasis-as-heading
warnings remain.
coji
changed the title
1 participant
Summary
message/wait_message(avoid AI prompt confusion)runId+ required payload and add explicit HTTP error casesadd-human-in-looptoadd-human-in-the-loopand update related specs/tasksuseHumanWaitswithrespondAPI and typed payload guidanceTest plan
openspec validate add-human-in-the-loop --strict --no-interactiveSummary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.