Epoch#1
Merged
Merged
Conversation
Code Review by Qodo
1. driver_main injects phase_instructions
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+62
to
+73
| # Inject phase_guidance for the initial phase so intake adapts to workflow scope | ||
| initial_guidance = workflow.phase_guidance.get(initial_phase, "") if workflow else "" | ||
|
|
||
| task = { | ||
| "role": "orchestrator", | ||
| "run_dir": run_dir, | ||
| "subagent_dir": subagent_dir, | ||
| "project_dir": app_state.project_dir, | ||
| "task_description": app_state.task_description, | ||
| "workflow": workflow_name, | ||
| "phase_instructions": initial_guidance, # scope framing for initial phase | ||
| } |
There was a problem hiding this comment.
1. driver_main injects phase_instructions 📘 Rule violation ⛨ Security
The driver injects initial_guidance into task["phase_instructions"] when spawning the orchestrator, which delivers phase guidance before the step-1 mechanism. This violates the requirement that phase guidance is delivered only through step-1 guidance after phase transitions are committed.
Agent Prompt
## Issue description
The driver passes `phase_instructions` into the orchestrator task at spawn time, which delivers phase guidance outside the step-1 guidance mechanism.
## Issue Context
Phase guidance should be injected only when `koan_complete_step` returns step-1 content for a committed phase (including the initial phase).
## Fix Focus Areas
- koan/driver.py[62-73]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+44
to
+73
| ROLE_PERMISSIONS: dict[str, frozenset[str]] = { | ||
| "intake": frozenset({ | ||
| "koan_complete_step", | ||
| "koan_ask_question", | ||
| "koan_request_scouts", | ||
| "edit", | ||
| "write", | ||
| }), | ||
| "scout": frozenset({ | ||
| "koan_complete_step", | ||
| }), | ||
| "orchestrator": frozenset({ | ||
| # Base set; actual permissions are phase-aware — see _check_orchestrator_permission | ||
| "koan_complete_step", | ||
| "koan_set_phase", | ||
| "koan_yield", | ||
| "koan_ask_question", | ||
| "koan_request_scouts", | ||
| "koan_request_executor", | ||
| "koan_select_story", | ||
| "koan_complete_story", | ||
| "koan_retry_story", | ||
| "koan_skip_story", | ||
| "koan_memorize", | ||
| "koan_forget", | ||
| "koan_memory_status", | ||
| "koan_search", | ||
| "edit", | ||
| "write", | ||
| "bash", |
There was a problem hiding this comment.
2. write/edit not markdown-only 📘 Rule violation ⛨ Security
The PR grants LLM agents Write/Edit and the permission logic only path-scopes writes, without blocking .json (or other non-markdown) edits in the run directory. This breaks the required boundary that LLM-facing edits are markdown-only and must not touch driver-owned JSON state files like run-state.json or story state.json.
Agent Prompt
## Issue description
LLM agents are granted `write`/`edit` capability, and the current permission model only path-scopes writes to `run_dir` (and for orchestrator), without restricting file types. This allows modifying driver-owned JSON state files (e.g., `run-state.json`, `stories/*/state.json`) and other non-markdown artifacts, violating the markdown-only boundary.
## Issue Context
The compliance contract requires LLM-facing edits to be limited to markdown artifacts and forbids direct JSON state edits by agents.
## Fix Focus Areas
- koan/subagent.py[106-110]
- koan/lib/permissions.py[44-88]
- koan/lib/permissions.py[283-306]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+259
to
+268
| # bash always allowed for non-orchestrator roles. | ||
| if tool_name == "bash": | ||
| return {"allowed": True, "reason": None} | ||
|
|
||
| # brief-generation step 1 (Read) is read-only — phase-aware gate. | ||
| if current_phase == "brief-generation" and current_step == 1 and tool_name in STEP_1_BLOCKED_TOOLS: | ||
| return { | ||
| "allowed": False, | ||
| "reason": ( | ||
| f"{tool_name} is not available during the Read step (step 1). " |
There was a problem hiding this comment.
3. Non-orchestrator bash always allowed 📘 Rule violation ⛨ Security
check_permission() allows bash for all non-orchestrator roles regardless of phase, and the scout tool whitelist includes Bash. This violates the requirement that bash be phase-restricted (execution/implementation-validation) and not callable in other phases.
Agent Prompt
## Issue description
`bash` is permitted for non-orchestrator roles in all phases, and the scout CLI whitelist includes `Bash`, enabling shell access during planning phases.
## Issue Context
The compliance model requires `bash` be restricted to execution and implementation-validation phases.
## Fix Focus Areas
- koan/lib/permissions.py[259-268]
- koan/subagent.py[106-110]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+448
to
+450
| run = Path(st.run_dir).resolve() | ||
| target = (run / req_path).resolve() | ||
| if not str(target).startswith(str(run)): |
There was a problem hiding this comment.
4. Artifact path escape 🐞 Bug ⛨ Security
api_artifact_content() uses a raw string prefix check (startswith) to enforce run-directory containment, which is not a path-boundary-safe check. A crafted artifact path can resolve to a sibling path whose absolute path shares the run directory prefix and still pass the guard, enabling unintended file reads outside the run directory.
Agent Prompt
### Issue description
`api_artifact_content` tries to prevent path traversal by checking `str(target).startswith(str(run))`. This is not a safe containment check because it does not enforce a path boundary; paths like `/.../run_backup/...` can incorrectly pass when `run` is `/.../run`.
### Issue Context
The route uses `{path:path}`, so the captured parameter can include `/` and `..` segments. The code resolves the path, but resolution alone does not guarantee containment; the guard must do a boundary-aware containment check.
### Fix Focus Areas
- koan/web/app.py[440-467]
- koan/web/app.py[1104-1112]
### Suggested fix
- Replace the `startswith` guard with a boundary-safe check:
- Prefer:
- `try: target.relative_to(run)` (deny on `ValueError`), or
- `if not target.is_relative_to(run): ...` (Python 3.9+).
- Keep the `.resolve()` call, but ensure the containment check is done after resolving.
- Add a small unit test to confirm `/api/artifacts/../<run_prefix>_other/file` is rejected.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
[WiP]