feat: add goal-brief launch flow with server-owned prompt and goal-authoring tools - #79
Closed
chriswritescode-dev wants to merge 2 commits into
Closed
feat: add goal-brief launch flow with server-owned prompt and goal-authoring tools#79chriswritescode-dev wants to merge 2 commits into
chriswritescode-dev wants to merge 2 commits into
Conversation
Contributor
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughThis PR adds session-scoped goal briefs, a ChangesGoal brief authoring and execution
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant GoalAgent
participant GoalBriefsRepo
participant ForgeExecutionDialog
participant TUIClient
participant AttachHook
participant AuditorLoop
GoalAgent->>GoalBriefsRepo: goal-write stores session goal brief
ForgeExecutionDialog->>GoalBriefsRepo: loadLaunchSpec selects stored brief
ForgeExecutionDialog->>TUIClient: execute goal launch spec
TUIClient->>AttachHook: create server-owned goal workspace
AttachHook->>AuditorLoop: attach goal session and dispatch audit prompt
AuditorLoop->>AttachHook: persist loop state and completion routing
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/tui-client.ts (1)
336-349: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDefault TUI launches now lose the implicit 50-iteration cap.
StampingmaxIterations: policy.maxIterationshere bypasses the attach-side fallback, andresolveLoopLaunchPolicy()currently returns0whenloop.defaultMaxIterationsis unset. That changes TUI-originated loops from capped to unbounded for existing setups that relied on the old default. Consider preserving the previous cap unlessloop.defaultMaxIterationsis explicitly configured.🤖 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 `@src/utils/tui-client.ts` around lines 336 - 349, The forgeLoop construction in the TUI launch path should preserve the historical 50-iteration default when loop.defaultMaxIterations is unset. Update the maxIterations value supplied by resolveLoopLaunchPolicy or the surrounding forgeLoop initialization so an explicit configuration remains authoritative while an unset value falls back to 50, without bypassing attach-side behavior.
🧹 Nitpick comments (7)
test/hooks/forge-session-attach.test.ts (2)
1397-1397: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAccidental line join — the test body's first statement sits on the declaration line.
✏️ Proposed formatting fix
- test('attach hook prefers inline planText over stored plan when both are available', async () => { const plansRepoGetForSession = vi.fn().mockReturnValue({ content: 'STALE_PRIOR_PLAN_TEXT' })+ test('attach hook prefers inline planText over stored plan when both are available', async () => {+ const plansRepoGetForSession = vi.fn().mockReturnValue({ content: 'STALE_PRIOR_PLAN_TEXT' })🤖 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 `@test/hooks/forge-session-attach.test.ts` at line 1397, Reformat the test declaration for “attach hook prefers inline planText over stored plan when both are available” so its body begins on the following line, keeping the existing plansRepoGetForSession setup and test behavior unchanged.
149-151: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a
configoverride onbuildHookDepsover post-hocas anymutation.Both new tests patch
execDeps.configafter construction; threading it through the overrides object keeps the fixture the single place that shapesexecDeps.Also applies to: 192-192
🤖 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 `@test/hooks/forge-session-attach.test.ts` around lines 149 - 151, Update the test fixtures around buildHookDeps so the server-side config is supplied through its overrides argument when constructing execDeps, rather than mutating execDeps.config afterward via as any. Apply the same change to both affected tests, keeping the loop.defaultMaxIterations values unchanged.test/agents.test.ts (1)
229-247: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant with the older assertion at Lines 202-207.
architect and architect-auto keep plan-authoring tools but deny goal-writefully subsumesarchitect agents retain plan-authoring tools. Consider dropping the older test to keep one source of truth.🤖 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 `@test/agents.test.ts` around lines 229 - 247, Remove the redundant older test asserting that architect agents retain plan-authoring tools, since the existing “architect and architect-auto keep plan-authoring tools but deny goal-write” test already covers that behavior. Keep the broader goal-write exclusion test unchanged.test/goal-briefs-repo.test.ts (1)
5-17: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winHand-rolled
goal_briefsDDL can drift from the shipped migration.This duplicates
src/storage/migrations/144_create_goal_briefs.sql; if that migration changes (extra column, constraint, index), these tests keep passing against a stale shape. Prefer bootstrapping the real schema (astest/utils/tui-stored-plan.test.tsdoes viaopenForgeDatabase) or a shared test-db helper.🤖 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 `@test/goal-briefs-repo.test.ts` around lines 5 - 17, Update createTestDb() to bootstrap the schema through the shipped migration or shared test database helper, such as openForgeDatabase, instead of maintaining hand-written goal_briefs DDL. Ensure the tests use the same schema and migrations as production.test/goal-brief-launch-flow.test.ts (1)
402-403: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWall-clock timings make these two tests flake-prone under CI load.
A 200ms ack timeout and a 30ms insert delay inside a 5000ms window are tight enough that a slow/loaded runner can invert either outcome. Consider raising the timeout budget (or driving the insert deterministically before the poll rather than via
setTimeout).Also applies to: 441-442, 479-479
🤖 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 `@test/goal-brief-launch-flow.test.ts` around lines 402 - 403, Adjust the timing-sensitive tests around the FORGE_TUI_ATTACH_ACK_TIMEOUT_MS and FORGE_TUI_ATTACH_ACK_POLL_MS setup to avoid CI flakiness: increase the acknowledgement timeout budget and ensure the delayed insert occurs deterministically before polling, replacing reliance on tight setTimeout scheduling where applicable. Preserve the intended success and timeout outcomes in the affected tests.test/utils/tui-stored-plan.test.ts (1)
26-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTemp DB files are never deleted, and the open/close boilerplate repeats in every test.
Each test creates
<tmpdir>/forge-tui-stored-plan-<uuid>.db(plus-wal/-shm) and leaves it behind. A smallwithTempForgeDb(fn)helper that seeds, closes, andrmSyncs in afinallywould remove the repeated try/catch and keep tmpdir clean.Also applies to: 90-96
🤖 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 `@test/utils/tui-stored-plan.test.ts` around lines 26 - 39, Add a shared withTempForgeDb helper in the stored-plan test file that creates the temporary database, runs the supplied callback, closes the database, and removes the database plus WAL/SHM files in a finally block. Refactor the tests around the existing database setup, including the case near lines 90–96, to use this helper and eliminate repeated try/catch cleanup while preserving each test’s assertions.src/loop/runtime-prompt.ts (1)
56-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrectness confirmed; consider extracting the duplicated variant-inclusion condition.
The auditor and code branches use the same multi-clause condition shape (
model != null || (explicitOrResolvedModel == null && !modelFailed)) with different variable names. Extracting it into one named helper would make the "why" easier to verify at a glance in future edits.♻️ Proposed extraction
+function shouldIncludeVariant(variant: string | undefined, attemptModel: string | undefined, resolvedModel: string | undefined, modelFailed: boolean): boolean {+ if (!variant) return false+ return attemptModel != null || (resolvedModel == null && !modelFailed)+}+ export function createPromptDispatch(deps: PromptDispatchDeps): PromptDispatch {- ...(model ? { auditorModel: model } : {}),- ...(input.variant && (model != null || (auditorModel == null && !freshState.modelFailed))- ? { auditorVariant: input.variant }- : {}),+ ...(model ? { auditorModel: model } : {}),+ ...(shouldIncludeVariant(input.variant, model, auditorModel, freshState.modelFailed)+ ? { auditorVariant: input.variant }+ : {}),- ...(model ? { model } : {}),- ...(input.variant && (model != null || (effectiveModel == null && !freshState.modelFailed))- ? { variant: input.variant }- : {}),+ ...(model ? { model } : {}),+ ...(shouldIncludeVariant(input.variant, model, effectiveModel, freshState.modelFailed)+ ? { variant: input.variant }+ : {}),Also applies to: 83-86
🤖 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 `@src/loop/runtime-prompt.ts` around lines 56 - 59, Extract the duplicated variant-inclusion condition used by the auditor and code branches into one clearly named helper, preserving the existing checks for an explicit/resolved model and the corresponding model-failure flag. Replace both inline conditions around the auditor and code variant assignments with the helper while keeping the existing input.variant guard and output behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/api/_media/tools.md`:
- Line 80: Update the content field description in the goal-write documentation
table to state that required headings are recommended and validated with
warnings, rather than asserting they must be present. Keep the existing
restrictions on “## Phase” headings and “<!-- forge-section -->” markers
unchanged.
In `@docs/tools.md`:
- Around line 70-82: The goal-write argument documentation should state that
required headings are recommended and missing headings produce warnings rather
than rejection. Update the content description in the goal-write section from
mandatory wording to equivalent “should contain” wording, while retaining the
rejection rule for ## Phase headings and forge-section markers.
In `@README.md`:
- Line 115: Update the agent-count wording from “three user-facing agents” to
“four user-facing agents” in README.md lines 115-115 and docs/api/README.md
lines 118-118, keeping the surrounding agent descriptions unchanged.
In `@src/agents/goal.ts`:
- Around line 12-21: Update the goal agent tool configuration around the
`tools.exclude` list to block the default bash/shell capability, including
non-sandbox sessions. Ensure `opencode-goal` either has shell access disabled
entirely or uses a read-only sandbox, while preserving the existing edit and
planning tool exclusions.
In `@src/prompts/agents/goal.md`:
- Line 45: Update the Goal heading description in goal.md to use “what the user
wants to achieve” instead of “what the user wants achieved,” leaving the rest of
the description unchanged.
---
Outside diff comments:
In `@src/utils/tui-client.ts`:
- Around line 336-349: The forgeLoop construction in the TUI launch path should
preserve the historical 50-iteration default when loop.defaultMaxIterations is
unset. Update the maxIterations value supplied by resolveLoopLaunchPolicy or the
surrounding forgeLoop initialization so an explicit configuration remains
authoritative while an unset value falls back to 50, without bypassing
attach-side behavior.
---
Nitpick comments:
In `@src/loop/runtime-prompt.ts`:
- Around line 56-59: Extract the duplicated variant-inclusion condition used by
the auditor and code branches into one clearly named helper, preserving the
existing checks for an explicit/resolved model and the corresponding
model-failure flag. Replace both inline conditions around the auditor and code
variant assignments with the helper while keeping the existing input.variant
guard and output behavior unchanged.
In `@test/agents.test.ts`:
- Around line 229-247: Remove the redundant older test asserting that architect
agents retain plan-authoring tools, since the existing “architect and
architect-auto keep plan-authoring tools but deny goal-write” test already
covers that behavior. Keep the broader goal-write exclusion test unchanged.
In `@test/goal-brief-launch-flow.test.ts`:
- Around line 402-403: Adjust the timing-sensitive tests around the
FORGE_TUI_ATTACH_ACK_TIMEOUT_MS and FORGE_TUI_ATTACH_ACK_POLL_MS setup to avoid
CI flakiness: increase the acknowledgement timeout budget and ensure the delayed
insert occurs deterministically before polling, replacing reliance on tight
setTimeout scheduling where applicable. Preserve the intended success and
timeout outcomes in the affected tests.
In `@test/goal-briefs-repo.test.ts`:
- Around line 5-17: Update createTestDb() to bootstrap the schema through the
shipped migration or shared test database helper, such as openForgeDatabase,
instead of maintaining hand-written goal_briefs DDL. Ensure the tests use the
same schema and migrations as production.
In `@test/hooks/forge-session-attach.test.ts`:
- Line 1397: Reformat the test declaration for “attach hook prefers inline
planText over stored plan when both are available” so its body begins on the
following line, keeping the existing plansRepoGetForSession setup and test
behavior unchanged.
- Around line 149-151: Update the test fixtures around buildHookDeps so the
server-side config is supplied through its overrides argument when constructing
execDeps, rather than mutating execDeps.config afterward via as any. Apply the
same change to both affected tests, keeping the loop.defaultMaxIterations values
unchanged.
In `@test/utils/tui-stored-plan.test.ts`:
- Around line 26-39: Add a shared withTempForgeDb helper in the stored-plan test
file that creates the temporary database, runs the supplied callback, closes the
database, and removes the database plus WAL/SHM files in a finally block.
Refactor the tests around the existing database setup, including the case near
lines 90–96, to use this helper and eliminate repeated try/catch cleanup while
preserving each test’s assertions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 17bffdc9-8916-4736-809b-3d898cd97eba
📒 Files selected for processing (71)
AGENTS.mdREADME.mddocs/agents-and-commands.mddocs/api/README.mddocs/api/_media/agents-and-commands.mddocs/api/_media/architecture.mddocs/api/_media/loop-system.mddocs/api/_media/tools.mddocs/architecture.mddocs/loop-system.mddocs/modules.mddocs/tools.mdsrc/agents/architect-auto.tssrc/agents/architect.tssrc/agents/auditor.tssrc/agents/code.tssrc/agents/feature-splitter.tssrc/agents/goal.tssrc/agents/index.tssrc/agents/types.tssrc/config.tssrc/constants/loop.tssrc/hooks/forge-session-attach.tssrc/index.tssrc/loop/runtime-prompt.tssrc/prompts/agents/goal.mdsrc/prompts/commands/goal.mdsrc/services/execution.tssrc/storage/index.tssrc/storage/migrations/144_create_goal_briefs.sqlsrc/storage/migrations/index.tssrc/storage/repos/goal-briefs-repo.tssrc/tools/goal-authoring.tssrc/tools/index.tssrc/tools/plan-authoring.tssrc/tools/session-write-guard.tssrc/tools/types.tssrc/tui.tsxsrc/tui/execute-plan-panel.tsxsrc/utils/goal-brief.tssrc/utils/loop-helpers.tssrc/utils/plan-execution.tssrc/utils/session-launch-spec.tssrc/utils/tui-client.tssrc/utils/tui-loop-store.tssrc/utils/tui-remote-launch.tssrc/workspace/classify-stale.tstest/agents.test.tstest/config-commands.test.tstest/config.test.tstest/constants/loop.test.tstest/goal-brief-launch-flow.test.tstest/goal-briefs-repo.test.tstest/hooks/forge-session-attach.test.tstest/loop-helpers.test.tstest/loop-permission-ruleset.test.tstest/loop/runtime.test.tstest/plan-execution.test.tstest/prompts/loader.test.tstest/services/attach-loop.test.tstest/services/execution.forge-loop-extra.test.tstest/storage-migrations.test.tstest/tools/goal-authoring.test.tstest/tui-client.loop-error.test.tstest/utils/goal-brief.test.tstest/utils/session-launch-spec.test.tstest/utils/tui-client-loop-inline-plan.test.tstest/utils/tui-client-stored-plan.test.tstest/utils/tui-client-warp-flow.test.tstest/utils/tui-remote-launch.test.tstest/utils/tui-stored-plan.test.ts
| | Argument | Description | | ||
| |---|---| | ||
| | `content` | Goal brief markdown. Must contain the required headings; must not contain `## Phase` headings or `<!-- forge-section -->` markers. | |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the required-heading contract.
goal-write persists briefs with missing-heading warnings, so “Must contain” is inaccurate. Say headings are recommended/validated with warnings instead.
🤖 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 `@docs/api/_media/tools.md` at line 80, Update the content field description in
the goal-write documentation table to state that required headings are
recommended and validated with warnings, rather than asserting they must be
present. Keep the existing restrictions on “## Phase” headings and “<!--
forge-section -->” markers unchanged.
Comment on lines
+70
to
+82
| ### `goal-write` | ||
| Writes the session-scoped **goal brief** that seeds a goal loop, the goal-mode counterpart of `plan-write`. Available to the `goal` agent only; denied in `code`, `auditor`, `auditor-loop`, `feature-splitter`, `architect`, and `architect-auto` sessions, and inside any running loop or audit session. Denied when the session owns a running loop, the same shared guard `plan-authoring.ts` uses. | ||
| Reports missing required headings as warnings, and rejects `## Phase` headings or `<!-- forge-section -->` markers without writing. A brief with heading warnings still persists so the agent can inspect the report and correct it. Accepts `append` to grow the brief incrementally. On success the brief is persisted to the session-scoped `goal_briefs` row and the tool returns a structure report with line/character counts and warnings. | ||
| Arguments: | ||
| | Argument | Description | | ||
| |---|---| | ||
| | `content` | Goal brief markdown. Must contain the required headings; must not contain `## Phase` headings or `<!-- forge-section -->` markers. | | ||
| | `append` | Append to the existing stored brief instead of replacing it. Two newlines are inserted between the existing content and the new fragment. Creates the brief when none exists. | | ||
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify that missing headings produce warnings, not rejection.
Line 74 says incomplete briefs persist with warnings, but Line 80 says content “must contain” the headings. The implementation only rejects plan structure; change this to “should contain” or equivalent.
Proposed documentation fix
-| `content` | Goal brief markdown. Must contain the required headings; must not contain `## Phase` headings or `<!-- forge-section -->` markers. |+| `content` | Goal brief markdown. Should contain the required headings; must not contain `## Phase` headings or `<!-- forge-section -->` markers. |📝 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
| ### `goal-write` | |
| Writes the session-scoped **goal brief** that seeds a goal loop, the goal-mode counterpart of `plan-write`. Available to the `goal` agent only; denied in `code`, `auditor`, `auditor-loop`, `feature-splitter`, `architect`, and `architect-auto` sessions, and inside any running loop or audit session. Denied when the session owns a running loop, the same shared guard `plan-authoring.ts` uses. | |
| Reports missing required headings as warnings, and rejects `## Phase` headings or `<!-- forge-section -->` markers without writing. A brief with heading warnings still persists so the agent can inspect the report and correct it. Accepts `append` to grow the brief incrementally. On success the brief is persisted to the session-scoped `goal_briefs` row and the tool returns a structure report with line/character counts and warnings. | |
| Arguments: | |
| | Argument | Description | | |
| |---|---| | |
| |`content`| Goal brief markdown. Must contain the required headings; must not contain `## Phase` headings or `<!-- forge-section -->` markers. | | |
| |`append`| Append to the existing stored brief instead of replacing it. Two newlines are inserted between the existing content and the new fragment. Creates the brief when none exists. | | |
| ### `goal-write` | |
| Writes the session-scoped **goal brief** that seeds a goal loop, the goal-mode counterpart of `plan-write`. Available to the `goal` agent only; denied in `code`, `auditor`, `auditor-loop`, `feature-splitter`, `architect`, and `architect-auto` sessions, and inside any running loop or audit session. Denied when the session owns a running loop, the same shared guard `plan-authoring.ts` uses. | |
| Reports missing required headings as warnings, and rejects `## Phase` headings or `<!-- forge-section -->` markers without writing. A brief with heading warnings still persists so the agent can inspect the report and correct it. Accepts `append` to grow the brief incrementally. On success the brief is persisted to the session-scoped `goal_briefs` row and the tool returns a structure report with line/character counts and warnings. | |
| Arguments: | |
| | Argument | Description | | |
| |---|---| | |
| |`content`| Goal brief markdown. Should contain the required headings; must not contain `## Phase` headings or `<!-- forge-section -->` markers. | | |
| |`append`| Append to the existing stored brief instead of replacing it. Two newlines are inserted between the existing content and the new fragment. Creates the brief when none exists. | |
🧰 Tools
🪛 LanguageTool
[grammar] ~81-~81: Ensure spelling is correct
Context: ...ored brief instead of replacing it. Two newlines are inserted between the existing conte...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@docs/tools.md` around lines 70 - 82, The goal-write argument documentation
should state that required headings are recommended and missing headings produce
warnings rather than rejection. Update the content description in the goal-write
section from mandatory wording to equivalent “should contain” wording, while
retaining the rejection rule for ## Phase headings and forge-section markers.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+12
to
+21
| tools: { | ||
| exclude: [ | ||
| 'write', 'edit', 'multiedit', 'apply_patch', 'patch', | ||
| 'plan', 'plan_enter', 'plan_exit', | ||
| 'execute-plan', 'execute-goal', | ||
| 'launch-group', 'group-status', 'group-cancel', | ||
| 'loop-cancel', 'loop-status', | ||
| 'review-write', 'review-delete', | ||
| 'plan-adjust', | ||
| ...PLAN_AUTHORING_TOOL_NAMES, |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'Files of interest:\n'
git ls-files 'src/agents/*''src/tools/*''src/services/*''src/constants/*'| sed -n '1,200p'printf'\nGoal agent file outline:\n'
ast-grep outline src/agents/goal.ts --view expanded ||trueprintf'\nSearch for shell tool registration and goal agent exclusions:\n'
rg -n --hidden --no-ignore-vcs 'shell|bash|execute-command|terminal|tool.*exclude|tools:\s*\{' src/agents src/tools src/services src/constants | sed -n '1,240p'Repository: chriswritescode-dev/opencode-forge
Length of output: 3778
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'src/agents/goal.ts:\n'
cat -n src/agents/goal.ts | sed -n '1,120p'printf'\nsrc/agents/types.ts:\n'
cat -n src/agents/types.ts | sed -n '1,180p'printf'\nsrc/tools/index.ts:\n'
cat -n src/tools/index.ts | sed -n '1,220p'printf'\nsrc/constants/loop.ts (relevant section):\n'
cat -n src/constants/loop.ts | sed -n '1,180p'Repository: chriswritescode-dev/opencode-forge
Length of output: 14263
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'Shell/tool registration search:\n'
rg -n --hidden --no-ignore-vcs '\b(bash|shell|terminal|exec|command)\b' src | sed -n '1,260p'printf'\nTool permission usage search:\n'
rg -n --hidden --no-ignore-vcs 'buildLoopPermissionRuleset|buildAuditSessionPermissionRuleset|permissionRuleset|permission.*bash|permission.*shell' src | sed -n '1,260p'Repository: chriswritescode-dev/opencode-forge
Length of output: 50393
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'Goal prompt excerpt:\n'
cat -n src/prompts/agents/goal.md | sed -n '1,120p'printf'\nSearch for goal-specific permission or shell restrictions:\n'
rg -n --hidden --no-ignore-vcs 'opencode-goal|buildGoalAgent|goal-write|assertWritableSession|question:\s*''allow''|shell\.env|shell config|read-only|readonly' src | sed -n '1,260p'Repository: chriswritescode-dev/opencode-forge
Length of output: 50391
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'src/index.ts shell hook section:\n'
sed -n '240,300p' src/index.ts
printf'\nsrc/hooks/shell-env.ts:\n'
cat -n src/hooks/shell-env.ts | sed -n '1,140p'printf'\nSearch for shell permission rules or exclusions:\n'
rg -n --hidden --no-ignore-vcs "'shell'|\"shell\"|bash tool|shell.env|permission.*shell|exclude:.*shell|shell.*deny" src | sed -n '1,220p'Repository: chriswritescode-dev/opencode-forge
Length of output: 7182
Block shell writes for the goal agent. The edit-tool exclusions still leave the default bash/shell path available, and non-sandbox sessions fall through to the host shell. That makes the “read-only” constraint prompt-only; disable shell access for opencode-goal or route it through a read-only sandbox.
🤖 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 `@src/agents/goal.ts` around lines 12 - 21, Update the goal agent tool
configuration around the `tools.exclude` list to block the default bash/shell
capability, including non-sandbox sessions. Ensure `opencode-goal` either has
shell access disabled entirely or uses a read-only sandbox, while preserving the
existing edit and planning tool exclusions.
| ## Goal Brief Format | ||
| The brief MUST contain exactly these four `##` headings, and MUST NOT contain any others: | ||
| - `## Goal` — What the user wants achieved and why it matters. The single, self-contained outcome statement the implementing loop will work from. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the goal-heading wording.
Change “what the user wants achieved” to “what the user wants to achieve” for grammatical correctness.
🧰 Tools
🪛 LanguageTool
[style] ~45-~45: The double modal “wants achieved” is nonstandard (only accepted in certain dialects). Consider “to be achieved”.
Context: ...hers: - ## Goal — What the user wants achieved and why it matters. The single, self-co...
(NEEDS_FIXED)
🤖 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 `@src/prompts/agents/goal.md` at line 45, Update the Goal heading description
in goal.md to use “what the user wants to achieve” instead of “what the user
wants achieved,” leaving the rest of the description unchanged.
Source: Linters/SAST 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 freeto 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
Adds a goal-brief launch flow that lets the TUI execution dialog and the
execute-goaltool launch a goal loop from a server-owned initial prompt.Introduces the
goalagent, thegoalcommand, thegoal-writeauthoringtool, the
goal_briefsstorage store + migration, the session-launch-specresolution used by the dialog, and the TUI/attach wiring (including a
server-owned initial prompt for goal specs and goal-loop runtime support).
Also closes two deferred pr-review findings from the prior loop pass and
re-attaches the verified ledger to the new HEAD.
Key changes
resolveLoopLaunchPolicy(singleresolution point for
loop.enabledandloop.defaultMaxIterations).ForgeLoopExtra.maxIterationsadded;handleStartLoop/handleStartGoalresolve iterations through the helper.launchTuiLoopenforcesloop.enabled,stamps
maxIterationsonto the forgeLoop envelope, and (local launches)waits for the running loop row on the shared forge DB before reporting
success via
waitForLoopRowAcknowledgement.connectForgeProjectforwards
pluginConfig/awaitAttachAck.pluginConfigto honor thelaunch policy; stays fire-and-forget (cannot observe the remote DB).
maxIterationsfirst, falling back to the server-side policy.new
goalagent and command.goal-writetool via the sharedassertWritableSessionguard insrc/tools/session-write-guard.ts.src/storage/repos/goal-briefs-repo.ts -
goal_briefsstore.brief validation and dialog launch-spec resolution.
policy fallback, loop policy unit, tui warp-flow envelope shape.
Deferred follow-ups (pr-review ledger)
TUI/remote launches honor
loop.enabledand stampmaxIterations; theattach hook honors the stamped value with a server-side policy fallback.
running loop row before reporting success; remote launches remain
fire-and-forget because the cross-process handshake for remote DB
observation is an architectural addition outside this PR. The open
finding is re-attached to HEAD on
refs/notes/pr-review.Validation
pnpm typecheck && pnpm lint && pnpm test && pnpm build- all green(172 test files, 2876 tests).
Summary by CodeRabbit
New Features
/goalworkflow andgoalagent for authoring and launching briefs.goal-writeand updatedexecute-goalto launch from stored briefs.Bug Fixes
Documentation