sudocode init #3939
sudocode init #3939
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a sudocode workspace with gitignore and config, introduces issues and spec metadata, adds a Conditional Workflow Branching spec and related issues, and updates AGENTS.md with spec/issue management guidance. Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(245,250,255)
participant Engine as WorkflowEngine
participant Store as Persistence
participant Block as ConditionalBlock
participant Eval as BranchEvaluator
participant Next as NextBlockResolver
end
Engine->>Block: execute(current_block_label, context)
Block->>Eval: evaluate branches in order (criteria / default)
activate Eval
Eval-->>Block: branch_result (matched / default / none)
deactivate Eval
alt matched_branch
Block->>Next: resolve matched_branch.next_block_label
Next-->>Engine: next_block_label
else default_branch
Block->>Next: resolve default.next_block_label
Next-->>Engine: next_block_label
else no_branch / cycle_detected
Block-->>Engine: error / terminate
end
Engine->>Store: persist branch decision (block, branch_id, next_label)
Store-->>Engine: ack
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 667374b in 2 minutes and 44 seconds. Click for details.
- Reviewed
40lines of code in4files - Skipped
1files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .sudocode/.gitignore:2
- Draft comment:
Confirm that the 'issues/' pattern is intended to ignore only subdirectories. Ensure this pattern doesn’t inadvertently ignore files like issues.jsonl in the same folder. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. .sudocode/.gitignore:3
- Draft comment:
Verify that the 'specs/' ignore pattern is scoped as intended. Specs are managed via a JSONL file rather than a directory, so confirm if this pattern should remain or be refined. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. .sudocode/issues.jsonl:1
- Draft comment:
The datetime format in issues.jsonl (e.g., '2025-11-06 19:12:15') differs from the ISO8601 format used in specs.jsonl. Consider standardizing to a single format for consistency. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. AGENTS.md:96
- Draft comment:
Typo: In 'make sure to created references', 'created' should be 'create'. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_9WvtvcNXGlXDeo0Z
You can customize 
|
|
||
| # Guidelines | ||
| This project uses sudocode for spec and issue management. Make sure to update issue status when working on an issue and close the issue when done with work. | ||
| When writing specs and creating issues that refer to specs, make sure to created references to bi-directionally link issues and specs. Refer to spec and issue content to gather more context about your current task. When editing spec or issue content, you can use your provided MVP tools but you can also update their contents directly by modifying the markdown files in .sudocode/specs/ and .sudocode/issues. |
There was a problem hiding this comment.
There's a minor typo: 'created references' should be 'create references'. Also, the guidelines mention updating markdown files in .sudocode/specs/ and .sudocode/issues, yet the managed files are JSONL. Consider clarifying the file paths/formats.
| When writing specs and creating issues that refer to specs, make sure to created references to bi-directionally link issues and specs. Refer to spec and issue content to gather more context about your current task. When editing spec or issue content, you can use your provided MVP tools but you can also update their contents directly by modifying the markdown files in .sudocode/specs/ and .sudocode/issues. | |
| When writing specs and creating issues that refer to specs, make sure to create references to bi-directionally link issues and specs. Refer to spec and issue content to gather more context about your current task. When editing spec or issue content, you can use your provided MVP tools but you can also update their contents directly by modifying the markdown files in .sudocode/specs/ and .sudocode/issues. |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.sudocode/.gitignore(1 hunks).sudocode/config.json(1 hunks).sudocode/issues.jsonl(1 hunks).sudocode/specs.jsonl(1 hunks)AGENTS.md(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-22T03:08:53.406Z
Learnt from: CR
Repo: Skyvern-AI/skyvern PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T03:08:53.406Z
Learning: Check existing issues, reference docs, provide reproduction steps, and be specific when seeking help
Applied to files:
AGENTS.md
📚 Learning: 2025-09-22T03:08:53.406Z
Learnt from: CR
Repo: Skyvern-AI/skyvern PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T03:08:53.406Z
Learning: Reference related issues in PRs using "Fixes #123" or "Closes #123" and include clear descriptions, docs updates, passing tests, and at least one approval
Applied to files:
AGENTS.md
🪛 LanguageTool
AGENTS.md
[grammar] ~95-~95: Ensure spelling is correct
Context: ...ehavior # Guidelines This project uses sudocode for spec and issue management. Make sur...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run tests and pre-commit hooks
🔇 Additional comments (3)
.sudocode/.gitignore (1)
1-4: LGTM!The ignore patterns are well-aligned with the sudocode configuration. The distinction between tracked JSONL files (
.sudocode/issues.jsonl,.sudocode/specs.jsonl) and ignored directories (.sudocode/issues/,.sudocode/specs/) for generated markdown content is clear and appropriate..sudocode/config.json (1)
1-11: LGTM!The worktree configuration is sensible with conservative defaults. The storage path aligns with
.gitignore, and theautoCreateBranches: truewithautoDeleteBranches: falsecombination provides safety during initial rollout..sudocode/issues.jsonl (1)
1-4: LGTM!The issue entries are well-structured with appropriate priorities. The high-priority security evaluation (i-6deh, priority 1) and proper bidirectional linking to spec
s-j4juusing[[s-j4ju]]notation demonstrate good issue management practices.
.sudocode/specs.jsonl
Outdated
| @@ -0,0 +1 @@ | |||
| {"id":"s-j4ju","uuid":"340b6315-ae19-4604-83f4-8d5a0ca2d740","title":"Block-Level Cache Key Templates","file_path":"specs/block_level_cache_key_templates.md","content":"## Overview\nIntroduce optional cache key templating at the workflow block level so that cached code/artifacts can vary per block instance using workflow parameters and prior block outputs. This augments the existing workflow-level cache key without breaking existing scripts.\n\nMy edit\n\n## Goals\n- Allow each block definition to specify an optional `cache_key_template` string that resolves with workflow/block context.\n- Ensure script generation, execution, persistence, and regeneration use the resolved block cache key so multiple variants can coexist.\n- Expose the setting in the workflow editor UI alongside existing cache controls.\n\n## Non-Goals\n- Changing how workflow-level cache keys are defined.\n- Modifying database schema beyond adding the new field to serialized models.\n- Redesigning broader caching invalidation policies.\n\n## Requirements\n### Backend API & Models\n- Extend all block models (`skyvern/forge/sdk/workflow/models/block.py`) to include `cache_key_template: str | None = None`, mirroring `disable_cache`.\n- Surface the field through:\n - YAML/REST schemas (`skyvern/schemas/workflows.py` and fern/OpenAPI definitions).\n - Generated client types (Python, TypeScript) and any DTO transforms.\n - Codegen inputs (`transform_workflow_run_to_code_gen_input`) so regenerated scripts keep templates.\n- Rendering rules for an executing block:\n 1. Render the workflow-level cache key (if present) using existing logic; otherwise fall back to `workflow.workflow_permanent_id`.\n 2. If the block has a `cache_key_template`, render it with a context containing:\n - Workflow parameters (same as current workflow-level rendering).\n - `WorkflowRunContext` block metadata and recorded outputs for completed blocks.\n 3. If evaluation succeeds and yields a non-empty string, set `block_cache_key = f\"{base_key}:{rendered_block_key}\"`.\n 4. If rendering fails or produces an empty string, log a warning and fall back to `cache_key or label` (current behaviour) to avoid breaking execution.\n- Update utilities (`_render_template_with_label` and helpers) to provide the enriched context and return both rendered string and context for reuse.\n- Script generation (`generate_script.py`) must emit decorators using the resolved block cache key so cached functions are registered under the new value.\n- Execution helpers in `script_service.py` should request cached functions with the resolved key and store workflow run blocks using that key when creating/updating cached script mappings.\n- Adjust regeneration logic (`_regenerate_script_block_after_ai_fallback`) and `workflow_script_service` retrieval so cached scripts are looked up by the new block key, ensuring per-block variants coexist.\n\n### Frontend\n- Add a text input for \"Cache Key Template\" to block configuration panels next to \"Disable Cache\" for all block types that support caching.\n- Display helper text indicating templates may reference workflow parameters and previous block outputs.\n- Persist the new field through editor state → API payload → save operations.\n- Optionally provide basic validation (e.g., warn if unmatched `{`/`}`) but allow submission to keep backend authoritative.\n\n### Compatibility & Validation\n- Because the new field defaults to `None`, existing workflows/scripts remain valid without migration; ensure serializers handle missing data.\n- Regenerate API clients / fern SDKs so downstream code sees the new property.\n- Validate key length before use; if longer than backend/storage limits (to confirm), consider hashing or truncating with warning.\n- Ensure log messages avoid leaking sensitive template data when render failures occur.\n\n## Testing\n- Unit tests for template rendering covering workflow params, block outputs, fallback cases, and error handling.\n- Integration test: create workflow with multiple blocks using distinct cache key templates, run twice with different parameter values, verify separate cached scripts.\n- Frontend e2e/regression to confirm template field persists and renders in editor.\n\n## Open Questions\n1. Should we hash or otherwise normalize extremely long rendered keys rather than storing raw strings? (Investigate current limits.)\n2. Do we need admin tooling to list block-level cache key variants similar to existing workflow cache key views?\n3. Are there security/data-governance concerns with exposing prior block outputs to templates (e.g., secrets)? Document and mitigate if needed.\n","priority":2,"archived":0,"archived_at":null,"created_at":"2025-11-06 19:30:22","updated_at":"2025-11-06T19:40:45.455Z","parent_id":null,"parent_uuid":null,"relationships":[],"tags":[]} | |||
There was a problem hiding this comment.
Remove leftover editing text from spec content.
The spec content includes "My edit" text that appears to be a leftover from editing. This should be removed for clean documentation.
The text appears in the content field between the Overview and Goals sections.
🤖 Prompt for AI Agents
In .sudocode/specs.jsonl around lines 1 to 1, remove the leftover editing text
"My edit" from the content field of the JSON record (between the Overview and
Goals sections) so the spec reads cleanly; update the JSON string value to
exclude that substring and any extra blank line, ensure escaping/whitespace
remains valid, and save the cleaned record back to the file.
AGENTS.md
Outdated
| # Guidelines | ||
| This project uses sudocode for spec and issue management. Make sure to update issue status when working on an issue and close the issue when done with work. | ||
| When writing specs and creating issues that refer to specs, make sure to created references to bi-directionally link issues and specs. Refer to spec and issue content to gather more context about your current task. When editing spec or issue content, you can use your provided MVP tools but you can also update their contents directly by modifying the markdown files in .sudocode/specs/ and .sudocode/issues. |
There was a problem hiding this comment.
Clarify the relationship between JSONL and markdown files.
The guidelines mention editing "markdown files in .sudocode/specs/ and .sudocode/issues/," but this PR introduces JSONL files (.sudocode/specs.jsonl and .sudocode/issues.jsonl). Please clarify:
- Which format is the source of truth?
- Are markdown files generated from JSONL, or vice versa?
- When should users edit JSONL vs markdown files?
This will help avoid confusion about the correct workflow for managing specs and issues.
🧰 Tools
🪛 LanguageTool
[grammar] ~95-~95: Ensure spelling is correct
Context: ...ehavior # Guidelines This project uses sudocode for spec and issue management. Make sur...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In AGENTS.md around lines 94-96, clarify the relationship and workflow between
the new JSONL files (.sudocode/specs.jsonl and .sudocode/issues.jsonl) and the
existing markdown files (.sudocode/specs/ and .sudocode/issues/): state which
format is the canonical source of truth, describe whether markdown files are
generated from JSONL or the other way around, and provide explicit guidance on
when to edit JSONL vs when to edit markdown (e.g., edit JSONL for programmatic
changes and bulk imports, edit markdown for manual human-editable spec
refinements, and avoid editing generated files directly), and add a short
example workflow line showing how to update an issue/spec so contributors know
which file to modify.
wintonzheng
force-pushed
the
shu/sudocode_testing
branch
from
667374b to
bb249ff
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed bb249ff in 1 minute and 52 seconds. Click for details.
- Reviewed
30lines of code in3files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .sudocode/issues.jsonl:1
- Draft comment:
All issue entries were removed, leaving the file essentially empty. Confirm this was intentional, as the PR description mentions initializing with four open issues. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This comment is explicitly asking the PR author to "confirm this was intentional," which directly violates the rule: "Do NOT ask the PR author to confirm their intention, to explain, to double-check things, to ensure the behavior is intended." The comment is not suggesting a code change or pointing out a definite bug - it's asking for clarification. Even though there appears to be a discrepancy with the PR description, the rules explicitly state "Do NOT assume that the PR description should be filled out" and we shouldn't ask authors to explain their changes. The comment doesn't provide actionable feedback that would result in a code change. The comment might be catching a genuine mistake where the author accidentally deleted content they meant to keep, especially given the PR description mentions initializing with four issues. This could be valuable feedback preventing an unintended deletion. While it's possible this catches a mistake, the rules are explicit that we should not ask authors to confirm intentions or explain changes. If the deletion was accidental, it would likely be caught in other ways (tests, manual review by the author). The comment doesn't provide a definitive issue, just asks for confirmation. This comment should be deleted because it asks the PR author to confirm their intention, which directly violates the review rules. It doesn't provide actionable feedback or identify a definite bug.
2. .sudocode/specs.jsonl:2
- Draft comment:
The new 'Conditional Workflow Branching' spec is very comprehensive. Consider using consistent ordered numbering for the list items to enhance readability. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. AGENTS.md:96
- Draft comment:
Typo: 'created references' should be 'create references' for better clarity. - Reason this comment was not posted:
Comment was on unchanged code.
4. AGENTS.md:96
- Draft comment:
Typo detected: In the phrase "make sure to created references", 'created' should be 'create'. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_jojXRyTrJ6xDUMUg
You can customize
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
AGENTS.md (2)
96-96: Fix typo: "created" should be "create".The phrase "make sure to created references" contains a grammatical error.
Apply this fix:
-When writing specs and creating issues that refer to specs, make sure to created references to bi-directionally link issues and specs. Refer to spec and issue content to gather more context about your current task. When editing spec or issue content, you can use your provided MVP tools but you can also update their contents directly by modifying the markdown files in .sudocode/specs/ and .sudocode/issues. +When writing specs and creating issues that refer to specs, make sure to create references to bi-directionally link issues and specs. Refer to spec and issue content to gather more context about your current task. When editing spec or issue content, you can use your provided MVP tools but you can also update their contents directly by modifying the markdown files in .sudocode/specs/ and .sudocode/issues.
96-96: Clarify the relationship between JSONL files and markdown directories.The documentation mentions editing "markdown files in
.sudocode/specs/and.sudocode/issues/" but this PR introduces JSONL files (.sudocode/specs.jsonland.sudocode/issues.jsonl). The JSONL records containfile_pathfields pointing to markdown files (e.g.,"specs/block_level_cache_key_templates.md"), and the gitignore excludes thespecs/andissues/directories, suggesting these directories contain generated or temporary content.Please clarify the workflow:
- Are the JSONL files the source of truth (database/index) and markdown files generated from them?
- Should users edit the JSONL directly for programmatic changes and markdown files for content editing?
- When do changes in one format sync to the other?
This will help contributors understand which files to modify in different scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.sudocode/.gitignore(1 hunks).sudocode/config.json(1 hunks).sudocode/issues.jsonl(1 hunks).sudocode/specs.jsonl(1 hunks)AGENTS.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .sudocode/issues.jsonl
🚧 Files skipped from review as they are similar to previous changes (1)
- .sudocode/config.json
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-22T03:08:53.406Z
Learnt from: CR
Repo: Skyvern-AI/skyvern PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T03:08:53.406Z
Learning: Check existing issues, reference docs, provide reproduction steps, and be specific when seeking help
Applied to files:
AGENTS.md
📚 Learning: 2025-09-22T03:08:53.406Z
Learnt from: CR
Repo: Skyvern-AI/skyvern PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T03:08:53.406Z
Learning: Reference related issues in PRs using "Fixes #123" or "Closes #123" and include clear descriptions, docs updates, passing tests, and at least one approval
Applied to files:
AGENTS.md
🪛 LanguageTool
AGENTS.md
[grammar] ~95-~95: Ensure spelling is correct
Context: ... and Issue Management This project uses sudocode for spec and issue management. Make sur...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run tests and pre-commit hooks
🔇 Additional comments (2)
.sudocode/.gitignore (1)
1-4: LGTM!The gitignore exclusions are appropriate for sudocode tooling artifacts. The patterns correctly exclude runtime database files, managed directories, and worktree storage.
.sudocode/specs.jsonl (1)
1-1: LGTM!The Block-Level Cache Key Templates specification is comprehensive and well-structured. The JSONL format is correct, and the content covers all necessary sections including requirements, testing, and open questions. The previously flagged "My edit" text is no longer present.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed f0139ff in 3 minutes and 26 seconds. Click for details.
- Reviewed
120lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. specs/conditional_workflow_branching.md:8
- Draft comment:
Consider revising “if / elif / elif / else” to simply “if/elif/else” for clarity and to avoid redundancy. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 50% This is a specification document (markdown file), not code. The comment is purely about writing style - suggesting to remove one "elif" from the phrase "if / elif / elif / else". The author likely wrote it this way to emphasize that multiple elif branches are supported (not just one). Looking at line 47, the spec also uses "if/elif/elif/.../else" with ellipsis, which further suggests the intent is to show multiple elif clauses are possible. This is a very minor stylistic suggestion that doesn't affect functionality or clarity in a meaningful way. The rules state not to make comments that are "obvious or unimportant" and this falls into that category - it's a trivial wording preference on a spec document. Perhaps the author genuinely finds "if / elif / elif / else" confusing and the comment would improve readability. The suggestion is clear and actionable, even if minor. While the suggestion is clear, it's addressing a trivial stylistic choice in a specification document where the repetition appears intentional to convey that multiple elif branches are supported. This falls under "obvious or unimportant" comments that should be filtered out. The spec is internally consistent (see line 47 with similar notation), suggesting this is a deliberate documentation choice. This comment should be deleted. It's a minor stylistic suggestion on a specification document where the current wording appears intentional to emphasize support for multiple elif branches. This doesn't meet the bar for requiring a change.
2. specs/conditional_workflow_branching.md:35
- Draft comment:
Consider adding an example or a reference for the BranchCriteria DSL to clarify what evaluators/operators are expected. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. specs/conditional_workflow_branching.md:8
- Draft comment:
There's a typographical error in the description of the BranchCondition model: it says "if / elif / elif / else". This appears to be a redundant repetition of 'elif'. Consider removing the duplicate so it reads "if / elif / else". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment assumes "if / elif / elif / else" is a typo, but this appears to be intentional. The document is describing a branching system that supports multiple elif conditions, not just one. Line 47 says "if/elif/elif/.../else" with ellipsis, which clearly indicates multiple elif branches are supported. Line 67 also uses the same pattern. The repetition of "elif" on line 8 is consistent with this design and is meant to convey that you can have multiple elif branches, not just a single if-elif-else structure. This is not a typo - it's describing the actual semantics of the system being designed. Could the author have made a typo on line 8 and then propagated it throughout the document? Or could line 8 be using shorthand that's inconsistent with the more explicit "..." notation used elsewhere? While it's possible the typo was propagated, the use of "..." on line 47 strongly suggests the author intentionally wants to show multiple elif branches. The pattern "if/elif/elif/.../else" is a common way to document that a construct supports multiple elif clauses. Line 8's "if / elif / elif / else" is consistent with this intent, just without the ellipsis. This is describing the semantics of a conditional branching system that explicitly supports multiple conditions. This comment should be deleted. The "if / elif / elif / else" pattern is intentional and indicates that the BranchCondition model supports multiple elif branches, not just one. This is confirmed by line 47's use of "if/elif/elif/.../else" with ellipsis and the overall design of ordered branch evaluation.
Workflow ID: wflow_NLGEtKugOTxetMXo
You can customize
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
specs/conditional_workflow_branching.md (1)
65-70: Assess performance and maintainability of dynamic script-cache discovery.Line 69 describes reconciling "the executed block graph against the cached script store to discover newly visited labels," which implies that new branches discovered during runs are added to the cache after the fact. This could lead to:
- Cache staleness if a branch is rarely executed on some runs but frequently on others.
- Maintenance burden if the cache becomes fragmented across runs.
- Potentially expensive cache-hit logic if the system must check multiple cache keys per block label.
Consider whether it would be simpler and more performant to:
- Generate and cache all reachable branches upfront at workflow validation/publish time, or
- Document the cache invalidation and reconciliation strategy more explicitly (e.g., when does stale cache get pruned?).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.sudocode/specs.jsonl(1 hunks)specs/conditional_workflow_branching.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .sudocode/specs.jsonl
🧰 Additional context used
🪛 LanguageTool
specs/conditional_workflow_branching.md
[grammar] ~48-~48: Use a hyphen to join words.
Context: ...evaluators. Criteria must be side-effect free and return truthy/falsey values reg...
(QB_NEW_EN_HYPHEN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run tests and pre-commit hooks
🔇 Additional comments (3)
specs/conditional_workflow_branching.md (3)
101-105: Incomplete review: open questions have partial answers already in the spec.The snippet provided was incomplete—it omits Question 4 about sentinel values, which IS present in the file. More critically, prior sections already address these concerns:
- Q1 (entry_block_label): Line 58 already proposes the decision: "starting from a new
workflow.entry_block_label(defaults to the first block for legacy workflows)"—not truly open.- Q2 (BranchCriteria evaluation): Line 35 scopes this: "must be explicit about supported operators and provide hooks for invoking an LLM-based evaluator when configured"—partially resolved.
- Q4 (sentinel values): Line 31 already addresses the None concern: "
next_block_label: str | None...Noneindicates the workflow should terminate after the branch"—not unaddressed.- Q3 (analytics): Legitimately open; reasonable to defer.
The questions in the spec are not "unresolved design choices left for implementation"—they are areas flagged because design decisions have been proposed but not finalized. The review comment mischaracterizes their status and did not examine the prior context establishing these decisions.
Likely an incorrect or invalid review comment.
37-43: Clarify serialization implementation details during development phase.The review targets a design specification for an unimplemented feature (
next_block_labelfor conditional workflow branching). No code, migrations, or tests for this field exist in the codebase. The questions raised are valid architectural concerns for the implementation phase but cannot be verified against non-existent code:
- Serializer defaults enforcement (schema validation, runtime checks, or migrations) needs to be decided during implementation.
- Database schema constraints (NOT NULL, defaults) vs. application-level enforcement must be explicitly chosen.
- Round-trip serialization tests should be added when the feature is implemented.
These are appropriate clarifications to address when implementing the feature, not issues with the design spec itself.
55-63: Clarify the cycle detection strategy and its responsibility division in the specification.The specification states: "Detect cycles at validation time; runtime should guard against infinite loops by tracking visited blocks and aborting with a descriptive error if validation was bypassed." This dual approach is sound (validation-time detection is O(V+E) and runtime tracking adds negligible overhead), but the specification should explicitly clarify:
- Mandatory validation: State that validation-time cycle detection is mandatory before any workflow execution begins.
- Division of responsibility: Make clear that validation-time detection is the primary defense (using algorithms like DFS or topological sort at save/update time), while runtime visited-block tracking is a safety fallback only if validation is circumvented (e.g., data corruption, race conditions).
- Rationale for both: Expand the current "if validation was bypassed" clause to explain edge cases that necessitate runtime guards.
Consider adding a brief statement such as: "Validation-time detection uses a single, deterministic pass to ensure the workflow DAG is acyclic; runtime guards serve as a defensive layer if validation is bypassed due to unforeseen failures or data inconsistencies."
| ### Branch Evaluation Semantics | ||
|
|
||
| - Evaluate branch conditions top-to-bottom by `order` to mimic `if/elif/elif/.../else`. | ||
| - `criteria` evaluates inside an execution context containing workflow params, accumulated block outputs, system variables, and (optionally) results from LLM boolean evaluators. Criteria must be side-effect free and return truthy/falsey values regardless of the underlying evaluator. | ||
| - Exactly one branch fires per conditional block: | ||
| 1. Iterate ordered branches until one returns `True`. | ||
| 2. If none match, fall back to the branch where `is_default=True`. Validation must ensure exactly one default exists (or zero if the author wants a “drop out” when nothing matches). | ||
| 3. If no branch matches and no default exists, log a warning, mark the workflow as failed (configurable), and stop execution to avoid silent drops. | ||
| - The chosen branch’s `next_block_label` determines the next block. `None` means the workflow ends successfully after executing the conditional block. |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify branch evaluation fallback behavior and error handling.
The fallback logic when no branch matches (lines 49–52) is partially specified. You state "log a warning, mark the workflow as failed (configurable), and stop execution," but the configurability aspect is vague. What are the configuration options, and where/how is this setting defined? Additionally, should the error message or event be observable/loggable for monitoring failed branches?
Consider adding an explicit field or config section that defines the failure mode (e.g., failure_mode: "error" | "warn_and_terminate" | "warn_and_skip"), or clarify whether this is controlled at the workflow level, organization level, or conditionalblock level.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for the full specification file to understand context
fd -t f "conditional_workflow_branching" --exec cat {} \;
Length of output: 9856
🏁 Script executed:
#!/bin/bash
# Search for any configuration-related content in the specs directory
fd -t f -e md . specs/ 2>/dev/null | head -20Length of output: 39
🏁 Script executed:
#!/bin/bash
# Look for failure_mode, config, configuration patterns in spec files
rg -i "(failure_mode|configuration|config_options|error_handling)" specs/ -n -A2 -B2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for workflow-level or block-level configuration structure definitions
rg -i "(workflow.*config|block.*config|failure.*mode|error.*mode)" specs/ -n -C3Length of output: 3501
Define the failure mode configuration mechanism for unmatched branches.
The specification uses "mark the workflow as failed (configurable)" at line 52 but provides no definition of the configuration options, where this setting is controlled, or what failure modes are supported.
Add an explicit failure_mode field to BranchCondition or ConditionalBlock (e.g., failure_mode: "error" | "warn_and_terminate" | "warn_and_skip"), or document whether this is controlled at the workflow level, block level, or elsewhere. Clarify how the fallback behavior is serialized, configured by authors, and observable during execution for monitoring/debugging.
🧰 Tools
🪛 LanguageTool
[grammar] ~48-~48: Use a hyphen to join words.
Context: ...evaluators. Criteria must be side-effect free and return truthy/falsey values reg...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
In specs/conditional_workflow_branching.md around lines 45 to 53, the spec
references a configurable "mark the workflow as failed (configurable)" behavior
but doesn't define where or how that configuration lives; add an explicit
failure_mode configuration and document its semantics: introduce a failure_mode
field (either on ConditionalBlock to apply to the whole conditional or on
BranchCondition to allow per-branch overrides) with allowed values such as
"error" (fail workflow and emit error), "warn_and_terminate" (log warning and
stop without marking failed), and "warn_and_skip" (log warning and continue to
next block), specify the default (e.g., ConditionalBlock.failure_mode="error"),
require validation rules (only one of these values allowed, fallbacks when both
block- and branch-level settings exist), describe how this is serialized in the
workflow JSON/YAML, where authors set it (block-level by default, optional
per-branch override), and how execution surfaces it (structured logs/events with
a failure_mode field and metric tags for monitoring and clear audit messages
emitted when no branch matches).
| ### Branch Evaluation Semantics | ||
|
|
||
| - Evaluate branch conditions top-to-bottom by `order` to mimic `if/elif/elif/.../else`. | ||
| - `criteria` evaluates inside an execution context containing workflow params, accumulated block outputs, system variables, and (optionally) results from LLM boolean evaluators. Criteria must be side-effect free and return truthy/falsey values regardless of the underlying evaluator. |
There was a problem hiding this comment.
Fix grammar: use hyphen in "side-effect-free".
The phrase should be hyphenated as a compound adjective modifying "return truthy/falsey values".
Apply this correction:
- Criteria must be side-effect free and return truthy/falsey values regardless
+ Criteria must be side-effect-free and return truthy/falsey values regardless📝 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.
| - `criteria` evaluates inside an execution context containing workflow params, accumulated block outputs, system variables, and (optionally) results from LLM boolean evaluators. Criteria must be side-effect free and return truthy/falsey values regardless of the underlying evaluator. | |
| - `criteria` evaluates inside an execution context containing workflow params, accumulated block outputs, system variables, and (optionally) results from LLM boolean evaluators. Criteria must be side-effect-free and return truthy/falsey values regardless of the underlying evaluator. |
🧰 Tools
🪛 LanguageTool
[grammar] ~48-~48: Use a hyphen to join words.
Context: ...evaluators. Criteria must be side-effect free and return truthy/falsey values reg...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
In specs/conditional_workflow_branching.md around line 48, the phrase
"side-effect free" should be hyphenated as a compound adjective; update the
sentence to use "side-effect-free" (e.g., "...Criteria must be side-effect-free
and return truthy/falsey values...") so the grammar is correct.
f0139ff to
6a3e0dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
AGENTS.md (1)
94-96: Fix grammar and clarify JSONL vs markdown vs JSONL workflow.
- Line 96: “make sure to created references” → “make sure to create references”.
- The guidance mentions editing markdown files in
.sudocode/specs/and.sudocode/issues, but this PR introduces.sudocode/specs.jsonland.sudocode/issues.jsonl. Please explicitly state:
- Which files are the source of truth (JSONL vs markdown).
- Where markdown specs actually live (e.g.,
specs/conditional_workflow_branching.mdvs any generated.sudocode/specs/copies).- When contributors should edit JSONL directly vs using sudocode tools vs editing markdown.
A minimal text fix for the grammar piece could be:
-When writing specs and creating issues that refer to specs, make sure to created references to bi-directionally link issues and specs. +When writing specs and creating issues that refer to specs, make sure to create references to bi-directionally link issues and specs.specs/conditional_workflow_branching.md (2)
48-48: Hyphenate “side-effect-free” in branch criteria description.Minor wording fix for line 48:
- Criteria must be side-effect free and return truthy/falsey values regardless of the underlying evaluator. + Criteria must be side-effect-free and return truthy/falsey values regardless of the underlying evaluator.
47-52: Define how the “configurable” failure behavior is controlled when no branch matches.The text says “log a warning, mark the workflow as failed (configurable), and stop execution,” but it doesn’t specify:
- Where this configuration lives (workflow-level, ConditionalBlock-level, or global setting).
- What modes are supported (e.g., always error vs warn-and-terminate vs warn-and-skip).
- How this is serialized and surfaced in logs/metrics.
Consider introducing an explicit failure-mode configuration (e.g., a
failure_modefield onConditionalBlockwith documented options and defaults), and describe how it interacts with validation and observability.
🧹 Nitpick comments (2)
.sudocode/issues.jsonl (1)
1-4: Issues JSONL looks consistent; consider adding explicit spec relationships.The four issue records are well-structured and consistently reference the branching spec via
[[s-4bnl]]. If sudocode supports first-class links via therelationshipsfield, consider also adding explicit relationships (e.g., typed links to specs-4bnl) so tools don’t have to parse wiki-style references from free text to understand the connections..sudocode/specs.jsonl (1)
1-1: Spec metadata entry looks good; tags are available for future filtering.The
s-4bnlspec record is consistent (id, title, file_path, and content all line up, and the empty tags array is correctly typed). If you expect to query specs by type (e.g., “workflow”, “dag”), you might want to add a few tags here for easier filtering later, but it’s fine as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.sudocode/.gitignore(1 hunks).sudocode/config.json(1 hunks).sudocode/issues.jsonl(1 hunks).sudocode/specs.jsonl(1 hunks)AGENTS.md(1 hunks)specs/conditional_workflow_branching.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .sudocode/.gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- .sudocode/config.json
🧰 Additional context used
🪛 LanguageTool
specs/conditional_workflow_branching.md
[grammar] ~48-~48: Use a hyphen to join words.
Context: ...evaluators. Criteria must be side-effect free and return truthy/falsey values reg...
(QB_NEW_EN_HYPHEN)
AGENTS.md
[grammar] ~95-~95: Ensure spelling is correct
Context: ... and Issue Management This project uses sudocode for spec and issue management. Make sur...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run tests and pre-commit hooks
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
specs/conditional_workflow_branching.md (1)
1-106: Comprehensive and coherent DAG + conditional branching spec.The spec is well-structured and covers data modeling, execution semantics, serialization/back-compat, caching/script generation, validation, and testing in a way that should be implementable without major ambiguities, aside from the minor nits noted in other comments.
🚀 This PR initializes sudocode project management tooling by setting up configuration files, issue tracking, and specification management. It establishes a structured workflow for managing development tasks and specifications within the project, including four initial issues related to block-level cache key templates.
🔍 Detailed Analysis
Key Changes
.sudocode/config.jsonwith version 0.1.5 and worktree management settings including auto-branch creation and cleanup policies.sudocode/issues.jsonlwith four open issues focused on block-level cache key implementation, normalization strategies, tooling requirements, and security considerations.sudocode/specs.jsonlcontaining a comprehensive specification for "Block-Level Cache Key Templates" featureAGENTS.mdwith sudocode usage guidelines and bi-directional linking requirements between issues and specs.sudocode/.gitignoreto exclude cache files, issues, specs, and worktrees from version controlTechnical Implementation
flowchart TD A[sudocode init] --> B[Configuration Files] A --> C[Issue Tracking] A --> D[Specification Management] B --> B1[config.json - worktree settings] B --> B2[.gitignore - exclusions] C --> C1[issues.jsonl - 4 open issues] C1 --> C2[Cache key normalization] C1 --> C3[Tooling requirements] C1 --> C4[Security evaluation] C1 --> C5[Hello World placeholder] D --> D1[specs.jsonl - cache templates spec] D1 --> D2[Backend API requirements] D1 --> D3[Frontend UI changes] D1 --> D4[Testing strategy] A --> E[Documentation Update] E --> E1[AGENTS.md guidelines]Impact
Created with Palmier
Important
Initialize sudocode project management with configuration, issue tracking, and specification management for block-level cache key templates.
.sudocode/config.jsonfor version 0.1.5 with worktree management settings..sudocode/.gitignoreto exclude cache files, issues, specs, and worktrees..sudocode/issues.jsonlwith four open issues on block-level cache key implementation..sudocode/specs.jsonlfor "Block-Level Cache Key Templates" feature.AGENTS.mdwith sudocode usage guidelines and linking requirements.specs/conditional_workflow_branching.mdfor conditional workflow branching.This description was created by
for f0139ff. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Documentation
Chores