Harden work-package execution repair#175
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e74664fe9e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Joncallim
left a comment
Joncallim
left a comment
There was a problem hiding this comment.
Detailed execution-repair review. These three cases can still exhaust the worker or reject valid tiny-app output, so they should be closed before the PR is considered ready.
Joncallim
left a comment
Joncallim
left a comment
There was a problem hiding this comment.
Follow-up review found the same command-selected validation gap on the build path.
Joncallim
left a comment
Joncallim
left a comment
There was a problem hiding this comment.
Additional validation-heuristic findings from the follow-up pass.
|
ACP blocker fixed in |
Joncallim
left a comment
Joncallim
left a comment
There was a problem hiding this comment.
Final follow-up found one command-alias consistency issue.
Joncallim
left a comment
Joncallim
left a comment
There was a problem hiding this comment.
Fresh blocker-focused review found three remaining correctness and security gaps in the current head.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd1fa8af74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Final review completed on bfa2c40 with no additional actionable findings. All inline findings have commit-specific evidence replies and are resolved. Local evidence: focused executor/context suites 64/64, full web suite 852/852, lint, TypeScript, and production build. GitHub evidence: PR Contract Check, GitGuardian, Web CI, and Playwright E2E are green; merge state is CLEAN with zero unresolved review threads. |
…ruth
Respond to the 17 inline review comments on ADR 0009 and the guides.
ADR 0009:
- Add per-MCP delivery kind: catalog membership != deliverable. GitHub reads
have no bounded-context producer, so they classify as planning/health-gated
context, never an approvable bounded grant; only filesystem has a producer.
Preserve the current safe-read breadth via a documented SAFE_READ_SUPPLEMENT
migrated verbatim from SAFE_BETA_CAPABILITY_PATTERNS (no behavior change).
- Add a package-normalization phase (admitWorkPackageMcp): union prohibitions
package-wide (deny-wins on every decision) and evaluate MCP-aware subtasks
against normalized coverage (preserves the current broker semantics). [P0]
- Replace the projectGrantCovers boolean with a normalized EffectiveGrantState
(phase/status/source/mode/consumed/coveredCapabilities) so denial and recovery
live in the shared decision.
- Make the decision table total and precedence-ordered; store per-capability
classifications; define required-empty, unknown-capability, mixed-class, and
ask_user outcomes; represent class 'unknown' and 'unknown_legacy'.
- Introduce the McpAdmissionEvaluation envelope ({decision, source, health}) so
the grant-preview/validation/broker adapters stay shape-preserving.
- Do not back-derive grant modes for legacy artifacts (unknown_legacy; UI reads
live package metadata instead of inventing approved state).
- Specify approval's health snapshot: getProjectMcpOverview runs outside the
status-flip transaction and its checkedAt is persisted; narrow the parity
guarantee to "no missed approval-time block", not "never blocks later".
- Reframe the invariant around the canonical package evaluation; the filesystem
helper is a tested projection, not a co-equal source.
- Denied/withheld required filesystem grant holds the package in a recoverable
state (zero attempts, task not failed); one project-wide reconciliation routine
shared by both grant endpoints; later project always-allow supersedes an
earlier package-local denial.
- Evidence lifecycle: planned scope (pre-run) vs issued packet METADATA (post-run,
incl. failed runs); file contents stay prompt-only, never persisted.
- deferred_live_mcp is actionable: required -> revise_plan CTA; optional ->
warning. Persisted broker-block schema versioned (metadata.mcpBroker), with
retryability + primary recovery action aggregation over multiple blocks.
- Assign each edit to one slice with transitional exports; S4 depends on S2,
S5 depends on S4. Refresh executor anchors against main after #175
(mcpCapabilityList 1252 -> 1527, filesystem throw ~1515 -> 1790).
- Note the #43 re-scope (planning/overlay only; runtime-tool AC deferred).
Guides:
- operator-guide, wiki: mark approval/handoff MCP parity as target-state (lands
with S2), not current behavior; narrow the guarantee.
- developer-guide, roadmap: classifier is catalog + safe-read supplement, and a
bounded packet is delivered only where a producer exists.
Docs only; no code changes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ruth
Respond to the 17 inline review comments on ADR 0009 and the guides.
ADR 0009:
- Add per-MCP delivery kind: catalog membership != deliverable. GitHub reads
have no bounded-context producer, so they classify as planning/health-gated
context, never an approvable bounded grant; only filesystem has a producer.
Preserve the current safe-read breadth via a documented SAFE_READ_SUPPLEMENT
migrated verbatim from SAFE_BETA_CAPABILITY_PATTERNS (no behavior change).
- Add a package-normalization phase (admitWorkPackageMcp): union prohibitions
package-wide (deny-wins on every decision) and evaluate MCP-aware subtasks
against normalized coverage (preserves the current broker semantics). [P0]
- Replace the projectGrantCovers boolean with a normalized EffectiveGrantState
(phase/status/source/mode/consumed/coveredCapabilities) so denial and recovery
live in the shared decision.
- Make the decision table total and precedence-ordered; store per-capability
classifications; define required-empty, unknown-capability, mixed-class, and
ask_user outcomes; represent class 'unknown' and 'unknown_legacy'.
- Introduce the McpAdmissionEvaluation envelope ({decision, source, health}) so
the grant-preview/validation/broker adapters stay shape-preserving.
- Do not back-derive grant modes for legacy artifacts (unknown_legacy; UI reads
live package metadata instead of inventing approved state).
- Specify approval's health snapshot: getProjectMcpOverview runs outside the
status-flip transaction and its checkedAt is persisted; narrow the parity
guarantee to "no missed approval-time block", not "never blocks later".
- Reframe the invariant around the canonical package evaluation; the filesystem
helper is a tested projection, not a co-equal source.
- Denied/withheld required filesystem grant holds the package in a recoverable
state (zero attempts, task not failed); one project-wide reconciliation routine
shared by both grant endpoints; later project always-allow supersedes an
earlier package-local denial.
- Evidence lifecycle: planned scope (pre-run) vs issued packet METADATA (post-run,
incl. failed runs); file contents stay prompt-only, never persisted.
- deferred_live_mcp is actionable: required -> revise_plan CTA; optional ->
warning. Persisted broker-block schema versioned (metadata.mcpBroker), with
retryability + primary recovery action aggregation over multiple blocks.
- Assign each edit to one slice with transitional exports; S4 depends on S2,
S5 depends on S4. Refresh executor anchors against main after #175
(mcpCapabilityList 1252 -> 1527, filesystem throw ~1515 -> 1790).
- Note the #43 re-scope (planning/overlay only; runtime-tool AC deferred).
Guides:
- operator-guide, wiki: mark approval/handoff MCP parity as target-state (lands
with S2), not current behavior; narrow the guarantee.
- developer-guide, roadmap: classifier is catalog + safe-read supplement, and a
bounded packet is delivered only where a producer exists.
Docs only; no code changes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Summary
Fixes #174.
This hardens the work-package execution path so Forge can recover from common specialist model output failures before a package burns all worker attempts. It also removes the default ACP package-execution blocker after an operator configures an ACP provider and approves the task.
What Changed
FORGE_ACP_WORK_PACKAGE_EXECUTION=0as the explicit opt-out because ACP adapters are local processes and are not OS-confined by Forge.Root Cause
The previous executor caught some bad plans only after sandbox writes, treated several salvageable response wrappers as total parse failures, and let weak validation heuristics either reject legitimate tests or accept no-op output. ACP package execution also treated an unset environment variable as disabled even after the operator had configured an ACP provider and approved the task.
Validation
npm test -- --run __tests__/work-package-executor.test.ts __tests__/work-package-execution-context.test.ts(64 tests)npx tsc --noEmit --pretty falsenpm run lintnpm test(852 tests)npm run buildNotes
ACP adapters remain local processes without Forge OS confinement. Operators that do not accept that access must set
FORGE_ACP_WORK_PACKAGE_EXECUTION=0. When available, Frontend, Backend, and QA package execution should still use a stronger provider than zero-configqwen2.5-coder:7b.