Fix prompts - #1
Merged
Merged
Conversation
Signed-off-by: ainetx <viator@via-net.org>
Signed-off-by: ainetx <viator@via-net.org>
📝 WalkthroughWalkthroughRefactors studio workflows into unit-gated controllers with strict entry via RootSkillEntrypointBootstrap, hardens sub-agent dispatch/approvals, revises analyze/generate/plan phases and gates, updates shared gates, and enhances cache to resolve/persist default-branch snapshot metadata. Tests and requirements docs are updated accordingly. ChangesStudio protocol/routing + workflows + cache provenance
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
workflows/studio.md (1)
10-10: 💤 Low valueConsider configuring markdownlint for workflow files.
The markdownlint warning (MD041) about missing a top-level heading is flagged because line 10 starts with a code fence rather than a heading. However, this file follows the studio workflow format with YAML frontmatter followed by UNIT code blocks, which is the intended structure.
If this pattern is standard across workflow files, consider adding a markdownlint configuration to exempt workflow definition files from the first-line-heading rule:
{ "MD041": { "front_matter_title": "^\\s*name\\s*:" } }Or add
<!-- markdownlint-disable MD041 -->at the top of workflow files that use this structure.🤖 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 `@workflows/studio.md` at line 10, The MD041 warning is triggered because workflows/studio.md begins with a code fence instead of a top-level heading; to fix, either add a project-level markdownlint rule that relaxes MD041 for workflow files by setting the front_matter_title regex (e.g., configure "MD041": { "front_matter_title": "^\\s*name\\s*:" } in your markdownlint config) or add a per-file exemption by inserting <!-- markdownlint-disable MD041 --> at the top of workflows/studio.md; update the repository's markdownlint config or add the disable comment accordingly and re-run linting to confirm the warning is suppressed.workflows/analyze/phase-0.1-plan-escalation-gate.md (1)
18-20: ⚡ Quick winRemove unused states from STATE declaration.
The
PLANNER_ESCALATION_RESULTstate includesproceed_smallandproceed_medium_warn, but these states are no longer set anywhere in the DO block after the refactor (which removed the size-threshold path). Onlybypassedandescalatedare used now.Consider cleaning up the STATE declaration to only include valid states:
bypassed | escalated.🤖 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 `@workflows/analyze/phase-0.1-plan-escalation-gate.md` around lines 18 - 20, The STATE declaration for PLANNER_ESCALATION_RESULT still lists unused values; update the STATE spec so PLANNER_ESCALATION_RESULT only includes the actual runtime values used (bypassed and escalated) by removing proceed_small and proceed_medium_warn from the enumerated list and leaving the default as appropriate (e.g., unset) so it matches the DO block logic and prevents invalid states from being declared.workflows/generate/phase-1-collect.md (1)
58-86: 💤 Low valueConsider clarifying BLOCKED emission behavior.
The exhaustion handling emits BLOCKED status in two scenarios with slightly different wording:
- Lines 59-60: "EMIT BLOCKED status with current stored_proposed_inputs"
- Lines 80-82: "EMIT refreshed Inputs block" then "EMIT BLOCKED status with partial Inputs block"
The distinction between "current stored_proposed_inputs" vs "refreshed Inputs block + partial Inputs block" could be made clearer. Is "partial Inputs block" redundant with "refreshed Inputs block" on line 80, or is it a different representation?
Consider standardizing the emission format or adding a note explaining the difference between these two BLOCKED scenarios.
🤖 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 `@workflows/generate/phase-1-collect.md` around lines 58 - 86, The BLOCKED-emission wording is ambiguous between the two exhaustion branches (COLLECTOR_MAX_ITER, stored_proposed_inputs, refreshed Inputs block); standardize by always emitting the refreshed Inputs block first and then emitting a single BLOCKED status that explicitly references which representation is being included (e.g., "BLOCKED status with partial Inputs: <refreshed Inputs block>" or "BLOCKED status with stored_proposed_inputs snapshot"), or add a short clarifying note explaining that the first branch emits the stored_proposed_inputs snapshot while the later branch emits the refreshed Inputs block as the partial Inputs representation; update the two lines to use the same phrasing and include the symbol names stored_proposed_inputs and refreshed Inputs block for clarity.workflows/generate/phase-4-write.md (2)
127-132: ⚖️ Poor tradeoffClarify parallel gate-release tracking implementation constraints.
Lines 127-132 describe per-task gate-release records for parallel execution but don't specify how concurrent orchestrator instances or threads should coordinate these records to avoid race conditions. If multiple tasks dispatch simultaneously, there's potential ambiguity about who owns the gate state.
Consider adding implementation notes about synchronization requirements:
Each parallel task owns its own gate-release record: task_id, selected_author, released_at, reset_at, completion_status. CF_PHASE_GATE MUST be treated as released only for that task's dispatch window and MUST be reset for every task independently before the group is considered complete. + NOTE: Implementations with concurrent dispatch must ensure gate-release + records are thread-safe or use task-scoped isolation to prevent races.🤖 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 `@workflows/generate/phase-4-write.md` around lines 127 - 132, Clarify concurrent coordination for per-task gate-release records: state that when INLINE_FALLBACK == false and tasks in the same group dispatch in parallel, updates to the gate-release record (task_id, selected_author, released_at, reset_at, completion_status) and the CF_PHASE_GATE must be synchronized using atomic DB operations or distributed locks (e.g., row-level transactions, compare-and-set/optimistic locking, or short-lived leases) so two orchestrator instances cannot claim the same gate window; require that released_at be set via an atomic create-or-update guarded by task_id and a lease TTL, resets update reset_at and completion_status only if the lease is held or CAS succeeds, and document that gate is considered released only for the specific task_id and its valid lease window to avoid race conditions.
171-178: ⚡ Quick winGit command constraints may need shell-escape validation.
The git constraint strings at lines 171-178 embed commands and flags that will be displayed to users and potentially interpreted by shell contexts. While the constraints use
MUST NOTguidance, there's no validation that prevents injection if these strings are later used in automated scripts.Consider adding a note about safe handling:
git_constraint = exactly one matching block: commit: "You MAY `git add` files you wrote and create one commit at the end. Follow the CONTRIBUTING guide (provided) for commit message format. MUST NOT `git push`, `git reset`, `git rebase`, `git stash`, `git checkout --`." stage: "You MAY `git add` files you wrote. MUST NOT `git commit`, `git push`, `git reset`, `git rebase`, `git stash`, `git checkout --`." none: "MUST NOT run `git commit`, `git push`, `git reset`, `git rebase`, `git stash`, `git checkout --`, or `git add`. Leave changes as uncommitted, unstaged working-tree edits only." RULES: - git_commit_mode MUST be included - contributing_guide MUST be included (null when not found) - git_constraint MUST be included verbatim matching current GIT_COMMIT_MODE + - git_constraint text MUST be treated as user-facing guidance only; MUST NOT + be executed directly by orchestrator without validation🤖 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 `@workflows/generate/phase-4-write.md` around lines 171 - 178, The guidance strings for commit, stage, and none in workflows/generate/phase-4-write.md embed shell-like commands and should be treated as data, not executed; update the code-paths that render or consume the commit, stage, and none values to either (a) render them as verbatim/code blocks or escape shell metacharacters before display, and (b) validate/sanitize any place that might pass these strings to a shell by applying a shell-escaping function or rejecting use in shell contexts; locate usages of the keys commit, stage, and none and ensure they are never interpolated into exec/system calls without escaping or explicit allow-listing.workflows/generate/phase-5/index.md (1)
119-128: ⚡ Quick winDeterministic validation on "accept" may surprise users after MAX_ITER exhaustion.
Lines 119-128 introduce a deterministic validator run when the user chooses
acceptat the iteration cap. If the gate fails, the menu re-prompts (line 126-128). However, users choosingacceptlikely expect to exit immediately, especially after MAX_ITER iterations have already run. Forcing an additional validation pass could feel like an unexpected gate.Consider making the final validation optional or at least warning users:
accept -> + EMIT "Running final validation check before accepting..." RUN one deterministic validator pass through phase-5.1-det-gate.md ON gate PASS or validator-backed SKIPPED: SET loop_exit = "max-iter-stopped" SET remaining_findings = carry_forward CONTINUE workflows/generate/phase-5/phase-5.5-final.md ON gate FAIL: SURFACE validator findings to user + EMIT "Final validation found issues. Choose extend/stop, or re-accept to override." EMIT_MENU IterationCapMenu WAIT user.reply STOP_TURN🤖 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 `@workflows/generate/phase-5/index.md` around lines 119 - 128, The deterministic validator run in the accept flow (the RUN of phase-5.1-det-gate.md when handling loop_exit = "max-iter-stopped") should not be mandatory; update the accept/IterationCapMenu handling so that when the user selects accept it either skips the final deterministic gate or first shows an explicit warning/confirmation that a final validation will run. Concretely, change the ON gate PASS/FAIL branch logic tied to loop_exit = "max-iter-stopped" and IterationCapMenu so that an "accept" reply bypasses running phase-5.1-det-gate.md (carry remaining_findings forward and CONTINUE to phase-5.5-final.md) or, alternatively, insert a confirmation prompt before RUN phase-5.1-det-gate.md that surfaces the impact and requires explicit consent.
🤖 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 `@skills/studio/SKILL.md`:
- Around line 115-121: The cross-reference in skills/studio/SKILL.md should use
the actual unit name from sub-agent-dispatch.md; change the reference text
“Contract-read-and-use gate” to the unit identifier SubAgentContractReadGate so
it matches UNIT SubAgentContractReadGate in skills/studio/sub-agent-dispatch.md
and keep the existing reference to UNIT CanonicalRoutingPrecedenceState in
skills/studio/routing.md; ensure any mentions of dispatch-blocking rules list
the exact symbol SubAgentContractReadGate (not the informal phrase) wherever the
gate is referenced.
- Around line 37-40: Update the “MUST STOP on self-detected violation” sentence
in SKILL.md to explicitly require a pre-output self-check or else specify the
exact runtime abort capability: change the wording around the clause starting
with “MUST STOP on self-detected violation” so it either mandates that the agent
perform the self-detection check before emitting any response bytes (i.e., a
pre-output validation step), or otherwise describes the precise runtime
primitive needed to cancel and restart an in-progress generation (include
expected behavior, API/primitive name, and when it may be invoked). Ensure the
revised text replaces the ambiguous “discard that draft and restart” wording and
clearly states which option is required by the spec.
In `@workflows/analyze.md`:
- Line 191: Summary: The "OR" check is ambiguous and may halt on the wrong
missing file; update the logic/text so we only stop when the specific terminal
file required by the chosen path is missing. Change the statement to explicitly
require the terminal file for the selected path (use phase-4-output/index.md
when producing the Remediation Handoff menu, or phase-5-next-steps.md when
producing the Phase 5 next-steps/PASS path), and reword the line to: "IF the
required terminal file (phase-4-output/index.md for remediation handoff, or
phase-5-next-steps.md for PASS path) is not loadable: STOP and surface missing
file error before emitting final response." Ensure any code or load-check that
enforces this uses the chosen path to decide which filename to validate rather
than failing if either file is missing.
In `@workflows/generate/phase-1.5-author-plan.md`:
- Around line 94-98: Clarify what "explicit user mode selection" means for
Emergency local fallback: specify that INLINE_FALLBACK must be accompanied by an
explicit flag or enum value (e.g., allowLocalFallback: true or mode:
"LOCAL_FALLBACK") supplied by the caller in the request/CLI invocation, and
require that the caller documents this selection in their public API/CLI docs
and include an audit trail (who, when) in the instruction-file write request
metadata; update the text around INLINE_FALLBACK and "instruction-file writes"
to require this named flag/enum and metadata so implementations know exactly
where and how the selection must be provided and recorded.
In `@workflows/generate/phase-1.5/offer-dispatch.md`:
- Around line 202-218: The PlannerValidationFailureMenu handling can lead to
unbounded retries after RE-DISPATCH cf-generate-planner because the flow
re-emits the same menu on repeated validation failures; update the logic to
track and enforce a maximum retry count (e.g., attach a retry counter to the
workflow context when entering PlannerValidationFailureMenu), increment it on
each RE-DISPATCH attempt and, when the counter exceeds the limit, transition to
a terminal failure state (emit a distinct FailureTerminalMenu or stop the turn)
instead of re-presenting the menu; ensure the branch that sets
AUTHOR_EXECUTION_PLAN and continues to Phase15Handoff remains unchanged on
successful validation and reset or clear the counter on success.
In `@workflows/shared/explore-brainstorm-gate.md`:
- Around line 118-120: The workflow references an undeclared control variable
ad_hoc_search_attempted in ExploreBrainstormAction; add it to the workflow's
STATE (or as an explicit input field) with a clear default (e.g., false) and
ensure any place that performs an ad‑hoc search sets it to true before the gate
is evaluated; update the STATE declaration for ExploreBrainstormAction to
include ad_hoc_search_attempted:boolean (default false) and wire any ad‑hoc
search step to mutate that state so the IF branch has a real signal.
In `@workflows/shared/plan-escalation-gate.md`:
- Around line 38-54: The gate references INLINE_FALLBACK_PROBED but that state
is not declared and its check overlaps with the INLINE_FALLBACK==unset branch
making one branch effectively unreachable; add INLINE_FALLBACK_PROBED to the
STATE section (with a clear boolean default) and then either consolidate the two
probe branches into a single probe condition (use INLINE_FALLBACK_PROBED != true
to trigger workflows/shared/inline-fallback-probe.md) or make the branches
semantically distinct (e.g., use INLINE_FALLBACK==unset only for initial
unresolved state and INLINE_FALLBACK_PROBED!=true only when a probe has not yet
run) so that PlanEscalationGate, INLINE_FALLBACK, INLINE_FALLBACK_PROBED, and
NoNativeDispatchPlanHandoff logic are consistent and non-overlapping.
---
Nitpick comments:
In `@workflows/analyze/phase-0.1-plan-escalation-gate.md`:
- Around line 18-20: The STATE declaration for PLANNER_ESCALATION_RESULT still
lists unused values; update the STATE spec so PLANNER_ESCALATION_RESULT only
includes the actual runtime values used (bypassed and escalated) by removing
proceed_small and proceed_medium_warn from the enumerated list and leaving the
default as appropriate (e.g., unset) so it matches the DO block logic and
prevents invalid states from being declared.
In `@workflows/generate/phase-1-collect.md`:
- Around line 58-86: The BLOCKED-emission wording is ambiguous between the two
exhaustion branches (COLLECTOR_MAX_ITER, stored_proposed_inputs, refreshed
Inputs block); standardize by always emitting the refreshed Inputs block first
and then emitting a single BLOCKED status that explicitly references which
representation is being included (e.g., "BLOCKED status with partial Inputs:
<refreshed Inputs block>" or "BLOCKED status with stored_proposed_inputs
snapshot"), or add a short clarifying note explaining that the first branch
emits the stored_proposed_inputs snapshot while the later branch emits the
refreshed Inputs block as the partial Inputs representation; update the two
lines to use the same phrasing and include the symbol names
stored_proposed_inputs and refreshed Inputs block for clarity.
In `@workflows/generate/phase-4-write.md`:
- Around line 127-132: Clarify concurrent coordination for per-task gate-release
records: state that when INLINE_FALLBACK == false and tasks in the same group
dispatch in parallel, updates to the gate-release record (task_id,
selected_author, released_at, reset_at, completion_status) and the CF_PHASE_GATE
must be synchronized using atomic DB operations or distributed locks (e.g.,
row-level transactions, compare-and-set/optimistic locking, or short-lived
leases) so two orchestrator instances cannot claim the same gate window; require
that released_at be set via an atomic create-or-update guarded by task_id and a
lease TTL, resets update reset_at and completion_status only if the lease is
held or CAS succeeds, and document that gate is considered released only for the
specific task_id and its valid lease window to avoid race conditions.
- Around line 171-178: The guidance strings for commit, stage, and none in
workflows/generate/phase-4-write.md embed shell-like commands and should be
treated as data, not executed; update the code-paths that render or consume the
commit, stage, and none values to either (a) render them as verbatim/code blocks
or escape shell metacharacters before display, and (b) validate/sanitize any
place that might pass these strings to a shell by applying a shell-escaping
function or rejecting use in shell contexts; locate usages of the keys commit,
stage, and none and ensure they are never interpolated into exec/system calls
without escaping or explicit allow-listing.
In `@workflows/generate/phase-5/index.md`:
- Around line 119-128: The deterministic validator run in the accept flow (the
RUN of phase-5.1-det-gate.md when handling loop_exit = "max-iter-stopped")
should not be mandatory; update the accept/IterationCapMenu handling so that
when the user selects accept it either skips the final deterministic gate or
first shows an explicit warning/confirmation that a final validation will run.
Concretely, change the ON gate PASS/FAIL branch logic tied to loop_exit =
"max-iter-stopped" and IterationCapMenu so that an "accept" reply bypasses
running phase-5.1-det-gate.md (carry remaining_findings forward and CONTINUE to
phase-5.5-final.md) or, alternatively, insert a confirmation prompt before RUN
phase-5.1-det-gate.md that surfaces the impact and requires explicit consent.
In `@workflows/studio.md`:
- Line 10: The MD041 warning is triggered because workflows/studio.md begins
with a code fence instead of a top-level heading; to fix, either add a
project-level markdownlint rule that relaxes MD041 for workflow files by setting
the front_matter_title regex (e.g., configure "MD041": { "front_matter_title":
"^\\s*name\\s*:" } in your markdownlint config) or add a per-file exemption by
inserting <!-- markdownlint-disable MD041 --> at the top of workflows/studio.md;
update the repository's markdownlint config or add the disable comment
accordingly and re-run linting to confirm the warning is suppressed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf9af65f-e494-4f04-98e8-278779f50a54
📒 Files selected for processing (63)
.bootstrap/config/core.toml.cspell.json.gitignore.markdownlint.jsonrequirements/prompt-bug-finding.mdrequirements/prompt-engineering.mdrequirements/raw-input-overflow.mdrequirements/storytelling-phases.mdrequirements/storytelling-preferences.mdrequirements/storytelling-shared.mdrequirements/storytelling.mdskills/studio/SKILL.mdskills/studio/agents/cf-prompt-bug-finder.mdskills/studio/agents/cf-ralphex.mdskills/studio/agents/cf-semantic-reviewer-prompt.mdskills/studio/protocol.mdskills/studio/routing.mdskills/studio/sub-agent-dispatch.mdsrc/studio_proxy/cache.pytests/test_studio_proxy_cli.pytests/test_workflow_parsing.pytests/test_workflow_subagents_dispatch.pyworkflows/analyze.mdworkflows/analyze/context-budget.mdworkflows/analyze/phase-0-change-review-scope.mdworkflows/analyze/phase-0.1-plan-escalation-gate.mdworkflows/analyze/phase-2-det-gate.mdworkflows/analyze/phase-2.5-reviewer-plan.mdworkflows/analyze/phase-3-semantic.mdworkflows/analyze/phase-3-to-4-checkpoint.mdworkflows/analyze/phase-4-output/remediation-handoff.mdworkflows/analyze/preamble.mdworkflows/analyze/validation-criteria.mdworkflows/auto-config.mdworkflows/brainstorm.mdworkflows/explain.mdworkflows/explore.mdworkflows/generate.mdworkflows/generate/phase-0-dependencies.mdworkflows/generate/phase-0.7/panel-selection.mdworkflows/generate/phase-0.7/round-loop.mdworkflows/generate/phase-1-collect.mdworkflows/generate/phase-1.5-author-plan.mdworkflows/generate/phase-1.5/offer-dispatch.mdworkflows/generate/phase-1.5/state-contract.mdworkflows/generate/phase-3-summary.mdworkflows/generate/phase-4-write.mdworkflows/generate/phase-5/index.mdworkflows/generate/phase-5/phase-5.1-det-gate.mdworkflows/generate/phase-5/phase-5.2-semantic.mdworkflows/generate/phase-5/phase-5.3-findings.mdworkflows/help.mdworkflows/map.mdworkflows/pdsl.mdworkflows/plan.mdworkflows/plan/phase-1-assess.mdworkflows/plan/phase-3-compile.mdworkflows/shared/explore-brainstorm-gate.mdworkflows/shared/inline-fallback-probe.mdworkflows/shared/plan-escalation-gate.mdworkflows/shared/stop-token-policy.mdworkflows/studio.mdworkflows/workspace.md
Comment on lines
+37
to
+40
| - MUST STOP on self-detected violation: reply starts with completion, | ||
| creation, save/write, or artifact-draft delivery phrasing before the | ||
| required gate/workflow/refusal shape; discard that draft and restart with | ||
| an allowed first-response shape |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for evidence of draft-discard or self-monitoring patterns in the codebase
rg -nP --type=py -C3 '\b(self.detect|draft.discard|cancel.response|stop.generation|preempt.output)\b'Repository: constructorfabric/studio
Length of output: 50
Clarify enforcement for “self-detected violation” to avoid requiring mid-stream cancellation — The “MUST STOP on self-detected violation… discard that draft and restart” wording in skills/studio/SKILL.md (lines 37-40) implies the runtime must be able to cancel/replace output already in progress; a repo search for draft-discard/self-monitoring/cancel primitives found none in the targeted code paths, so the spec should explicitly require a pre-output self-check (before emitting any response bytes) or state the exact runtime capability needed to abort and restart mid-generation.
🤖 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 `@skills/studio/SKILL.md` around lines 37 - 40, Update the “MUST STOP on
self-detected violation” sentence in SKILL.md to explicitly require a pre-output
self-check or else specify the exact runtime abort capability: change the
wording around the clause starting with “MUST STOP on self-detected violation”
so it either mandates that the agent perform the self-detection check before
emitting any response bytes (i.e., a pre-output validation step), or otherwise
describes the precise runtime primitive needed to cancel and restart an
in-progress generation (include expected behavior, API/primitive name, and when
it may be invoked). Ensure the revised text replaces the ambiguous “discard that
draft and restart” wording and clearly states which option is required by the
spec.
Comment on lines
+115
to
+121
| - MUST treat routing.md § CanonicalRoutingPrecedenceState as the single | ||
| precedence authority for workflow entry, explain mode, workspace quick | ||
| commands, AGENTS prompt-asset order, and fallback dispatch state | ||
| - MUST NOT skip any of the three files | ||
| - MUST treat sub-agent-dispatch.md § Contract-read-and-use gate as | ||
| dispatch-blocking for every cf-* DISPATCH, PARALLEL_DISPATCH, | ||
| RE-DISPATCH, or inline fallback execution |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify CanonicalRoutingPrecedenceState in routing.md
rg -nP 'UNIT\s+CanonicalRoutingPrecedenceState' skills/studio/routing.md
# Verify Contract-read-and-use gate in sub-agent-dispatch.md
rg -nP '(Contract-read-and-use|SubAgentContractReadGate)' skills/studio/sub-agent-dispatch.mdRepository: constructorfabric/studio
Length of output: 356
🏁 Script executed:
#!/bin/bash
# Check exact/near-exact section labels referenced in the review comment
rg -n --fixed-string "CanonicalRoutingPrecedenceState" skills/studio/routing.md
rg -n --fixed-string "Contract-read-and-use" skills/studio/sub-agent-dispatch.md
rg -n --fixed-string "Contract-read-and-use gate" skills/studio/sub-agent-dispatch.md
rg -n --fixed-string "Contract-read-and-use gate" -S skills/studio/sub-agent-dispatch.md
rg -n 'UNIT\s+.*Contract.*Read.*Gate|ReadGate|Contract.*Read.*Gate|SubAgentContractReadGate' skills/studio/sub-agent-dispatch.mdRepository: constructorfabric/studio
Length of output: 2127
🏁 Script executed:
#!/bin/bash
# Search exact referenced literal phrases (no unsupported flags)
rg -n "CanonicalRoutingPrecedenceState" skills/studio/routing.md
rg -n "Contract-read-and-use gate" skills/studio/sub-agent-dispatch.md
rg -n "Contract-read-and-use" skills/studio/sub-agent-dispatch.md
rg -n "SubAgentContractReadGate" skills/studio/sub-agent-dispatch.mdRepository: constructorfabric/studio
Length of output: 356
🏁 Script executed:
#!/bin/bash
# Show the section around the unit label(s) to determine exact naming/text
nl -ba skills/studio/routing.md | sed -n '1,120p'
nl -ba skills/studio/sub-agent-dispatch.md | sed -n '120,220p'
nl -ba skills/studio/sub-agent-dispatch.md | sed -n '1,120p'Repository: constructorfabric/studio
Length of output: 191
🏁 Script executed:
#!/bin/bash
# Inspect exact labeled content around the referenced units
cat -n skills/studio/routing.md | sed -n '1,120p'
cat -n skills/studio/sub-agent-dispatch.md | sed -n '1,220p'
cat -n skills/studio/sub-agent-dispatch.md | sed -n '220,420p'Repository: constructorfabric/studio
Length of output: 26584
Update the sub-agent dispatch gate cross-reference name to SubAgentContractReadGate.
skills/studio/routing.mdcontainsUNIT CanonicalRoutingPrecedenceStateand its rules cover the single precedence/dispatch-state model for workflow entrypoints, explain proxying, workspace quick-command handling, AGENTS prompt-asset bootstrap order, and fallback dispatch gates.skills/studio/sub-agent-dispatch.mdblocks all relevant cf-* dispatches viaUNIT SubAgentContractReadGate(WHENincludesDISPATCH,PARALLEL_DISPATCH,RE-DISPATCH, and Mode B inline execution); however the file’s unit name isSubAgentContractReadGate, not a literalContract-read-and-use gateheader—align the cross-reference to the actual unit name.
🤖 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 `@skills/studio/SKILL.md` around lines 115 - 121, The cross-reference in
skills/studio/SKILL.md should use the actual unit name from
sub-agent-dispatch.md; change the reference text “Contract-read-and-use gate” to
the unit identifier SubAgentContractReadGate so it matches UNIT
SubAgentContractReadGate in skills/studio/sub-agent-dispatch.md and keep the
existing reference to UNIT CanonicalRoutingPrecedenceState in
skills/studio/routing.md; ensure any mentions of dispatch-blocking rules list
the exact symbol SubAgentContractReadGate (not the informal phrase) wherever the
gate is referenced.
| - IF {cf-studio-path}/.core/workflows/analyze/phase-4-output/index.md OR {cf-studio-path}/.core/workflows/analyze/phase-5-next-steps.md | ||
| is not loadable: | ||
| STOP and surface the missing file before emitting any final response | ||
| - IF phase-4-output/index.md OR phase-5-next-steps.md is not loadable: STOP and surface missing file before emitting any final response |
There was a problem hiding this comment.
Clarify the file-loading check logic.
The invariant states that responses must end with either the Remediation Handoff menu (requires phase-4-output/index.md) or the Phase 5 next-steps menu (requires phase-5-next-steps.md). However, Line 191 uses "OR" in a way that's ambiguous: "IF phase-4-output/index.md OR phase-5-next-steps.md is not loadable: STOP..."
This could be interpreted as "stop if either file is missing" (even the one you won't use) or "stop if the file you need is missing". The former causes false positives; the latter is correct but not clearly expressed.
Consider rewording to: "IF the required terminal file (phase-4-output/index.md for remediation handoff, or phase-5-next-steps.md for PASS path) is not loadable: STOP and surface missing file error before emitting final response."
🤖 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 `@workflows/analyze.md` at line 191, Summary: The "OR" check is ambiguous and
may halt on the wrong missing file; update the logic/text so we only stop when
the specific terminal file required by the chosen path is missing. Change the
statement to explicitly require the terminal file for the selected path (use
phase-4-output/index.md when producing the Remediation Handoff menu, or
phase-5-next-steps.md when producing the Phase 5 next-steps/PASS path), and
reword the line to: "IF the required terminal file (phase-4-output/index.md for
remediation handoff, or phase-5-next-steps.md for PASS path) is not loadable:
STOP and surface missing file error before emitting final response." Ensure any
code or load-check that enforces this uses the chosen path to decide which
filename to validate rather than failing if either file is missing.
Comment on lines
+94
to
+98
| - INLINE_FALLBACK=true still requires planner/author contract execution; it | ||
| is not permission for controller-local edits. | ||
| - Emergency local fallback for instruction-file writes requires an explicit | ||
| user mode selection documented by the caller; absent that selection, stop. | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Clarify "explicit user mode selection" requirement for emergency fallback.
Lines 96-98 state that emergency local fallback requires "an explicit user mode selection documented by the caller" but don't define what constitutes such a selection or where it should be documented. Without this clarity, implementations may interpret this requirement inconsistently.
📋 Proposed clarification
Consider adding a concrete definition:
- Emergency local fallback for instruction-file writes requires an explicit
- user mode selection documented by the caller; absent that selection, stop.
+ Emergency local fallback for instruction-file writes requires the caller to
+ document an explicit user-supplied flag (e.g., `--emergency-local-edit`) or
+ interactive confirmation captured in session state; absent that selection, stop.📝 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.
Suggested change
| - INLINE_FALLBACK=true still requires planner/author contract execution; it | |
| is not permission for controller-local edits. | |
| - Emergency local fallback for instruction-file writes requires an explicit | |
| user mode selection documented by the caller; absent that selection, stop. | |
| ``` | |
| - INLINE_FALLBACK=true still requires planner/author contract execution; it | |
| is not permission for controller-local edits. | |
| - Emergency local fallback for instruction-file writes requires the caller to | |
| document an explicit user-supplied flag (e.g., `--emergency-local-edit`) or | |
| interactive confirmation captured in session state; absent that selection, stop. |
🤖 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 `@workflows/generate/phase-1.5-author-plan.md` around lines 94 - 98, Clarify
what "explicit user mode selection" means for Emergency local fallback: specify
that INLINE_FALLBACK must be accompanied by an explicit flag or enum value
(e.g., allowLocalFallback: true or mode: "LOCAL_FALLBACK") supplied by the
caller in the request/CLI invocation, and require that the caller documents this
selection in their public API/CLI docs and include an audit trail (who, when) in
the instruction-file write request metadata; update the text around
INLINE_FALLBACK and "instruction-file writes" to require this named flag/enum
and metadata so implementations know exactly where and how the selection must be
provided and recorded.
Comment on lines
+202
to
+218
| LOAD {cf-studio-path}/.core/skills/studio/agents/cf-generate-planner.md | ||
| as the planner source contract | ||
| SYNTHESIZE final dispatch prompt from planner contract plus | ||
| SHARED_CONTEXT_PACK and the same inputs | ||
| IF planner source contract is not loaded, unreadable, ambiguous, or not | ||
| reflected in the final dispatch prompt: | ||
| FAIL per sub-agent-dispatch.md § Contract-read-and-use gate | ||
| FORBID re-dispatch | ||
| RE-DISPATCH cf-generate-planner with synthesized final prompt | ||
| RE-VALIDATE returned plan | ||
| IF validation passes: | ||
| SET AUTHOR_EXECUTION_PLAN = parsed author_plan JSON | ||
| CONTINUE Phase15Handoff | ||
| IF validation fails again: | ||
| EMIT_MENU PlannerValidationFailureMenu | ||
| WAIT user.reply | ||
| STOP_TURN |
There was a problem hiding this comment.
Potential infinite retry loop in planner validation failure handling.
Option 1 in PlannerValidationFailureMenu (lines 202-218) will re-dispatch the planner and, if validation fails again, re-emit the same menu (line 216-218). This creates a two-attempt limit, but the logic could be clearer about what happens after the second failure—it appears to loop back to the menu again, potentially allowing infinite retries.
🔁 Suggested clarification
Consider adding explicit retry count tracking or clarifying the terminal behavior:
1 ->
LOAD {cf-studio-path}/.core/skills/studio/agents/cf-generate-planner.md
as the planner source contract
SYNTHESIZE final dispatch prompt from planner contract plus
SHARED_CONTEXT_PACK and the same inputs
IF planner source contract is not loaded, unreadable, ambiguous, or not
reflected in the final dispatch prompt:
FAIL per sub-agent-dispatch.md § Contract-read-and-use gate
FORBID re-dispatch
RE-DISPATCH cf-generate-planner with synthesized final prompt
RE-VALIDATE returned plan
IF validation passes:
SET AUTHOR_EXECUTION_PLAN = parsed author_plan JSON
CONTINUE Phase15Handoff
IF validation fails again:
+ EMIT "Planner validation failed twice. Cancelling."
+ SET AUTHOR_PLAN_OFFER_RESOLVED = cancelled_planner_failure
+ SET AUTHOR_EXECUTION_PLAN = null
+ STOP current generate sub-flow
- EMIT_MENU PlannerValidationFailureMenu
- WAIT user.reply
- STOP_TURNAlternatively, document that the menu is intentionally re-presented to allow further retries.
🤖 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 `@workflows/generate/phase-1.5/offer-dispatch.md` around lines 202 - 218, The
PlannerValidationFailureMenu handling can lead to unbounded retries after
RE-DISPATCH cf-generate-planner because the flow re-emits the same menu on
repeated validation failures; update the logic to track and enforce a maximum
retry count (e.g., attach a retry counter to the workflow context when entering
PlannerValidationFailureMenu), increment it on each RE-DISPATCH attempt and,
when the counter exceeds the limit, transition to a terminal failure state (emit
a distinct FailureTerminalMenu or stop the turn) instead of re-presenting the
menu; ensure the branch that sets AUTHOR_EXECUTION_PLAN and continues to
Phase15Handoff remains unchanged on successful validation and reset or clear the
counter on success.
Comment on lines
+118
to
+120
| IF ad_hoc_search_attempted: | ||
| EMIT "Use cf-explorer sub-agent for required project-resource discovery." | ||
| STOP_TURN |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for ad_hoc_search_attempted definition or assignment
rg -n -C3 'ad_hoc_search_attempted' --type=mdRepository: constructorfabric/studio
Length of output: 602
🏁 Script executed:
rg -n 'ad_hoc_search_attempted' -S .Repository: constructorfabric/studio
Length of output: 148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="workflows/shared/explore-brainstorm-gate.md"
echo "=== STATE/initialization area (lines 1-70) ==="
nl -ba "$FILE" | sed -n '1,70p'
echo "=== Middle area around decision (lines 90-140) ==="
nl -ba "$FILE" | sed -n '90,140p'Repository: constructorfabric/studio
Length of output: 176
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="workflows/shared/explore-brainstorm-gate.md"
echo "=== STATE/initialization area (lines 1-70) ==="
awk 'NR>=1 && NR<=70 {printf "%d:%s\n", NR, $0}' "$FILE"
echo "=== Decision area around IF ad_hoc_search_attempted (lines 90-130) ==="
awk 'NR>=90 && NR<=130 {printf "%d:%s\n", NR, $0}' "$FILE"Repository: constructorfabric/studio
Length of output: 5285
Fix undeclared control variable ad_hoc_search_attempted in ExploreBrainstormAction.
workflows/shared/explore-brainstorm-gate.md checks IF ad_hoc_search_attempted: (lines 118-120), but ad_hoc_search_attempted is not declared in the STATE section and is the only occurrence in the repo. Define/initialize it (or model it as an explicit input/state field) so the workflow evaluation has a real signal for this branch.
🤖 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 `@workflows/shared/explore-brainstorm-gate.md` around lines 118 - 120, The
workflow references an undeclared control variable ad_hoc_search_attempted in
ExploreBrainstormAction; add it to the workflow's STATE (or as an explicit input
field) with a clear default (e.g., false) and ensure any place that performs an
ad‑hoc search sets it to true before the gate is evaluated; update the STATE
declaration for ExploreBrainstormAction to include
ad_hoc_search_attempted:boolean (default false) and wire any ad‑hoc search step
to mutate that state so the IF branch has a real signal.
Comment on lines
+38
to
+54
| IF INLINE_FALLBACK_PROBED != true: | ||
| RUN workflows/shared/inline-fallback-probe.md | ||
| CONTINUE PlanEscalationGate (re-evaluate after resolution) | ||
|
|
||
| IF SUB_AGENT_SESSION_APPROVED == true AND INLINE_FALLBACK == false: | ||
| CONTINUE SubAgentDecompositionBypass | ||
|
|
||
| IF SUB_AGENT_SESSION_APPROVED == true AND INLINE_FALLBACK == unset: | ||
| IF INLINE_FALLBACK == unset: | ||
| RUN workflows/shared/inline-fallback-probe.md | ||
| CONTINUE PlanEscalationGate (re-evaluate after resolution) | ||
|
|
||
| CONTINUE LegacySizeBasedEscalation | ||
| IF INLINE_FALLBACK == true OR host.supports_native_subagents == false: | ||
| CONTINUE NoNativeDispatchPlanHandoff | ||
|
|
||
| IF SUB_AGENT_SESSION_APPROVED != true: | ||
| RUN workflows/shared/inline-fallback-probe.md | ||
| CONTINUE PlanEscalationGate (re-evaluate after resolution) |
There was a problem hiding this comment.
Undefined state variable and potentially unreachable code.
Line 38 references INLINE_FALLBACK_PROBED but this variable is not declared in the STATE section (lines 19-25). This creates ambiguity about when this variable is set and how it differs from checking INLINE_FALLBACK == unset.
Additionally, the re-evaluation conditions appear to overlap:
- Lines 38-40: Check
INLINE_FALLBACK_PROBED != true→ run probe → continue - Lines 45-47: Check
INLINE_FALLBACK == unset→ run probe → continue
If INLINE_FALLBACK is unset, presumably INLINE_FALLBACK_PROBED would also not be true, making the first check catch all cases and rendering lines 45-47 unreachable.
🛠️ Proposed fix
Add INLINE_FALLBACK_PROBED to the STATE section and clarify the distinct conditions, or remove the redundant check:
STATE:
SUB_AGENT_SESSION_APPROVED: unset | true
scope: session
INLINE_FALLBACK: unset | true | false
scope: workflow_run
+ INLINE_FALLBACK_PROBED: unset | true
+ scope: workflow_run
ESCALATION_ESTIMATE: integer (lines)
scope: workflow_runThen ensure the logic differentiates the two checks, or consolidate to a single probe condition:
IF raw-input-overflow rule has already fired for direct prompt/provided-file
input over 500 lines:
EMIT raw-input-overflow plan-vs-stop choice (higher precedence — resolve first)
STOP_TURN
- IF INLINE_FALLBACK_PROBED != true:
+ IF INLINE_FALLBACK == unset:
RUN workflows/shared/inline-fallback-probe.md
CONTINUE PlanEscalationGate (re-evaluate after resolution)
IF SUB_AGENT_SESSION_APPROVED == true AND INLINE_FALLBACK == false:
CONTINUE SubAgentDecompositionBypass
- IF INLINE_FALLBACK == unset:
- RUN workflows/shared/inline-fallback-probe.md
- CONTINUE PlanEscalationGate (re-evaluate after resolution)
-
IF INLINE_FALLBACK == true OR host.supports_native_subagents == false:
CONTINUE NoNativeDispatchPlanHandoff🤖 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 `@workflows/shared/plan-escalation-gate.md` around lines 38 - 54, The gate
references INLINE_FALLBACK_PROBED but that state is not declared and its check
overlaps with the INLINE_FALLBACK==unset branch making one branch effectively
unreachable; add INLINE_FALLBACK_PROBED to the STATE section (with a clear
boolean default) and then either consolidate the two probe branches into a
single probe condition (use INLINE_FALLBACK_PROBED != true to trigger
workflows/shared/inline-fallback-probe.md) or make the branches semantically
distinct (e.g., use INLINE_FALLBACK==unset only for initial unresolved state and
INLINE_FALLBACK_PROBED!=true only when a probe has not yet run) so that
PlanEscalationGate, INLINE_FALLBACK, INLINE_FALLBACK_PROBED, and
NoNativeDispatchPlanHandoff logic are consistent and non-overlapping.
This was referenced Jun 1, 2026
Merged
Merged
Merged
This was referenced Jun 10, 2026
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
1 participant
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 by CodeRabbit
New Features
Improvements
/cf-planor stopping (local single-context continuation no longer available by default).Documentation