feat(epic): workspace-runtime 작업을 준비한다
This commit is contained in:
parent
a94002a19c
commit
9982278eca
24 changed files with 4826 additions and 0 deletions
|
|
@ -0,0 +1,154 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/07+04_workspace_catalog plan=0 tag=API milestone-task=workspace-binding -->
|
||||
|
||||
# Code Review Reference - API
|
||||
|
||||
> **[IMPLEMENTING AGENT — READ FIRST] Filling in this file is the mandatory final step of implementation.**
|
||||
> The task is NOT complete until every implementation-owned section below is filled in.
|
||||
> Complete the `Implementation Checklist`; the final checklist item is mandatory before saving.
|
||||
> Fill implementation-owned sections, then stop with active files in place and report ready for review.
|
||||
> Execute the plan's selected root cause, scope, files, and dependency decisions as written. Do not choose another owner, narrow/expand the write boundary, or replace a fix with another verification attempt.
|
||||
> If implementation is blocked, record the exact blocker, attempted commands/output, and resume condition only in implementation-owned evidence fields.
|
||||
> Do not ask the user directly, present choices, call user-input tools, create control-plane stop files, or classify the next state.
|
||||
> Finalization (`Code Review Result`, log rename, `complete.log`, archive moves, `Review-Only Checklist`) is review-agent-only, even after compaction/resume.
|
||||
> Follow the ownership table at the bottom of this file for which sections you own.
|
||||
|
||||
## Overview
|
||||
|
||||
date=2026-08-06
|
||||
task=m-iop-owned-single-request-agent-execution/07+04_workspace_catalog, plan=0, tag=API
|
||||
|
||||
## For the Review Agent
|
||||
|
||||
> **[REVIEW AGENT ONLY]** The finalization steps below are review-agent only. Implementing agents must not execute this section.
|
||||
|
||||
Compare implementation of each item against source files and verify that output in `Verification Results` matches code.
|
||||
Review completion means the following steps are finished:
|
||||
|
||||
1. Append verdict and `review_rework_count` / `evidence_integrity_failure` routing signals.
|
||||
2. Archive `CODE_REVIEW-cloud-G07.md` → `code_review_cloud_G07_0.log` and `PLAN-local-G06.md` → `plan_local_G06_0.log`.
|
||||
3. If PASS, write `complete.log` and move active task directory to `agent-task/archive/YYYY/MM/m-iop-owned-single-request-agent-execution/07+04_workspace_catalog/`. If WARN/FAIL, fully write the next filesystem state required by the code-review skill.
|
||||
4. If PASS, preserve the first-line `milestone-task=workspace-binding` metadata in `complete.log` and report it for runtime aggregation. Roadmap state evaluation belongs to `sync-milestone-workstate`.
|
||||
5. Check applicable `Review-Only Checklist` items at the final `.log` location before reporting.
|
||||
|
||||
---
|
||||
|
||||
## Implementation Item Completion
|
||||
|
||||
| Item | Status |
|
||||
|------|---------|
|
||||
| API-1 Add the approved workspace catalog schema | [ ] |
|
||||
| API-2 Compile catalog ownership and restart semantics | [ ] |
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Define and fail-closed validate the globally unique operator workspace catalog, closed operations, fixed command templates, Mac platform, and numeric/environment boundaries.
|
||||
- [ ] Preserve immutable workspace capabilities in `NodeStore`, expose exact-ref lookup, and classify workspace changes as restart-required.
|
||||
- [ ] Synchronize the config example, inner config contract, and provider/config-refresh living spec without claiming runtime execution.
|
||||
- [ ] Run dependency, focused race, package, vet, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Review-Only Checklist
|
||||
|
||||
> **[REVIEW AGENT ONLY]** Implementing agents must not modify this section.
|
||||
|
||||
- [ ] Append one verdict and verified routing signals to `Code Review Result`.
|
||||
- [ ] Verify findings and dimension assessment.
|
||||
- [ ] Archive this file to `code_review_cloud_G07_0.log` and the plan to `plan_local_G06_0.log`.
|
||||
- [ ] Verify the managed `.gitignore` block.
|
||||
- [ ] On PASS, write `complete.log`, preserve Milestone metadata, move this directory to the monthly archive, and retain the active parent while siblings remain.
|
||||
- [ ] On WARN/FAIL, write only the next state required by the code-review skill.
|
||||
|
||||
## Deviations from Plan
|
||||
|
||||
_Record deviations and rationale._
|
||||
|
||||
## Key Design Decisions
|
||||
|
||||
_Record implemented decisions._
|
||||
|
||||
## Reviewer Checkpoints
|
||||
|
||||
- Confirm presets contain only opaque refs; raw roots/templates remain operator config and private Node payload facts.
|
||||
- Confirm duplicate refs and every invalid boundary fail before runtime observation.
|
||||
- Confirm store access returns immutable copies and refresh cannot change a live workspace.
|
||||
- Confirm no protobuf, filesystem, command, or coordinator behavior was claimed here.
|
||||
|
||||
## Verification Results
|
||||
|
||||
Paste actual stdout/stderr for every command. If a command changes, record the replacement and reason under `Deviations from Plan`.
|
||||
|
||||
### 1. Dependency
|
||||
|
||||
`test -f agent-task/m-iop-owned-single-request-agent-execution/04+02_preset_refresh/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/04+02_preset_refresh/complete.log' | wc -l)" -eq 1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 2. Config race tests
|
||||
|
||||
`go test -race ./packages/go/config -run 'TestLoadEdgeWorkspaceCatalog' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 3. Store/refresh race tests
|
||||
|
||||
`go test -race ./apps/edge/internal/node ./apps/edge/internal/configrefresh -run 'Test(LoadFromConfig.*Workspace|NodeStore.*Workspace|ClassifyWorkspace)' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 4. Package regression
|
||||
|
||||
`go test ./packages/go/config ./apps/edge/internal/node ./apps/edge/internal/configrefresh -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 5. Vet
|
||||
|
||||
`go vet ./packages/go/config ./apps/edge/internal/node ./apps/edge/internal/configrefresh`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 6. Documentation search
|
||||
|
||||
`rg --sort path -n 'workspace_ref|workspaces|restart_required|darwin' configs/edge.yaml agent-contract/inner/edge-config-runtime-refresh.md agent-spec/runtime/provider-pool-config-refresh.md`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 7. Whitespace
|
||||
|
||||
`git diff --check`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
> **[IMPLEMENTING AGENT — BEFORE SAVING] Have you filled in every implementation-owned section?**
|
||||
> If anything is blank, go back and fill it in before saving this file.
|
||||
> Leave review-agent-only sections unchanged.
|
||||
|
||||
## Section Ownership
|
||||
|
||||
| Section | Owner | Note |
|
||||
|---------|-------|------|
|
||||
| Header comment, Overview, Review Agent Instructions | Fixed at stub creation | Implementing agent must not modify or execute these (archive, complete.log, and task-directory archive move are review-agent only) |
|
||||
| Archive Evidence Snapshot | Fixed at stub creation from plan when present | Implementing agent uses it as default prior-loop context; read only the specific archive files cited there when more detail is required |
|
||||
| Implementation Item Completion (item names) | Fixed at stub creation | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Implementation Checklist (item text/order) | Fixed at stub creation from plan | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Review-Only Checklist | Review agent only | Implementing agent must not modify or check this section |
|
||||
| Deviations from Plan, Key Design Decisions | Implementing agent | Replace placeholder text with actual content |
|
||||
| Reviewer Checkpoints | Fixed at stub creation | Pre-filled from plan |
|
||||
| Verification Results (section headings + commands) | Fixed at stub creation | Implementing agent fills in command output only; command changes require a `Deviations from Plan` entry |
|
||||
| Code Review Result | Review agent appends | Not included in stub |
|
||||
|
|
@ -0,0 +1,235 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/07+04_workspace_catalog plan=0 tag=API milestone-task=workspace-binding -->
|
||||
|
||||
# Operator-owned Mac Workspace Catalog
|
||||
|
||||
## For the Implementing Agent
|
||||
|
||||
Do not start until packet 04 has `complete.log`. Implement only the files in `Modified Files Summary`, run every verification command, fill the paired review stub with actual evidence, and leave finalization to the code-review skill. If blocked, record the exact evidence and resume condition in the review stub; do not ask the user or create control-plane artifacts.
|
||||
|
||||
## Background
|
||||
|
||||
The fixed-light preset carries only an opaque `workspace_ref`, but Edge has no operator-owned catalog that maps that reference to one Mac Node root and bounded file/command capabilities. This packet creates that source of truth without sending tool requests yet.
|
||||
|
||||
## Analysis
|
||||
|
||||
### Files Read
|
||||
|
||||
- `AGENTS.md`
|
||||
- `agent-ops/rules/project/rules.md`
|
||||
- `agent-ops/rules/common/rules-roadmap.md`
|
||||
- `agent-ops/rules/common/rules-agent-spec.md`
|
||||
- `agent-ops/rules/project/domain/edge/rules.md`
|
||||
- `agent-ops/rules/project/domain/platform-common/rules.md`
|
||||
- `agent-test/local/rules.md`
|
||||
- `agent-test/local/edge-smoke.md`
|
||||
- `agent-roadmap/priority-queue.md`
|
||||
- `agent-roadmap/phase/knowledge-tool-optimization-extension/PHASE.md`
|
||||
- `agent-roadmap/phase/knowledge-tool-optimization-extension/milestones/iop-owned-single-request-agent-execution.md`
|
||||
- `agent-roadmap/sdd/knowledge-tool-optimization-extension/iop-owned-single-request-agent-execution/SDD.md`
|
||||
- `packages/go/config/edge_types.go`
|
||||
- `packages/go/config/load.go`
|
||||
- `packages/go/config/edge_runtime_config_test.go`
|
||||
- `apps/edge/internal/node/store.go`
|
||||
- `apps/edge/internal/node/store_test.go`
|
||||
- `apps/edge/internal/configrefresh/classify.go`
|
||||
- `apps/edge/internal/configrefresh/node_runtime_classify_test.go`
|
||||
- `configs/edge.yaml`
|
||||
- `agent-contract/inner/edge-config-runtime-refresh.md`
|
||||
- `agent-spec/runtime/provider-pool-config-refresh.md`
|
||||
|
||||
### SDD Criteria
|
||||
|
||||
- The approved SDD maps `workspace-binding` to S04 and requires approved/denied workspace, foreign Node/path, and symlink-escape candidates to fail before execution.
|
||||
- This foundation supplies S04's operator catalog and refresh boundary. Runtime admission and filesystem containment remain in packets 08 and 10.
|
||||
- D03, D06, and D08 require a Mac Node-owned bounded executor and forbid caller-selected raw roots and reuse of provider wire.
|
||||
|
||||
### Verification Context
|
||||
|
||||
- Starting HEAD is `a94002a19c774b90160f87a99887b531f7d84015`; the worktree was clean before plan creation.
|
||||
- `go version go1.26.2 linux/arm64`, `go`, `make`, and `protoc` are available.
|
||||
- Fresh baseline tests passed for config, Edge node/config-refresh/service/transport, and Node transport/node/bootstrap packages.
|
||||
- Repository-native fallback is `LoadEdge`, `NodeStore`, restart classification, their unit tests, and the approved config contract/spec. No external runner is required.
|
||||
|
||||
### Test Coverage Gaps
|
||||
|
||||
- No config validates globally unique workspace refs, absolute non-root paths, the fixed `darwin` platform, closed operations, bounded sizes/timeouts, exact command templates, or environment-name allowlists.
|
||||
- `NodeStore` and refresh classification discard workspace ownership facts.
|
||||
|
||||
### Symbol References
|
||||
|
||||
- `NodeDefinition` is decoded by `LoadEdge` and compiled by `LoadFromConfig`.
|
||||
- `nodeKey` in config refresh must retain the new catalog so changes cannot be silently live-applied.
|
||||
- No existing symbol is renamed or removed.
|
||||
|
||||
### Split Judgment
|
||||
|
||||
- Stable contract: validated config plus `NodeStore.ResolveWorkspace` can independently PASS before wire/executor work.
|
||||
- Packet 04 owns preset refresh files first; this packet therefore depends on 04 and may edit them only after its completion.
|
||||
|
||||
### Scope Rationale
|
||||
|
||||
- Include catalog schema, validation, store lookup, restart classification, example comments, contract, and living spec.
|
||||
- Exclude protobuf, Node filesystem access, admission generation fencing, process execution, and coordinator integration.
|
||||
|
||||
### Final Routing
|
||||
|
||||
- `evaluation_mode=first-pass`; finalizer `finalize-task-policy.sh` in `pair` mode.
|
||||
- Build closures are true; scores 2/0/2/1/1 = G06; route `local-fit`, lane `local`, filename `PLAN-local-G06.md`.
|
||||
- Build signals: `large_indivisible_context=false`; positive risks `boundary_contract`, `structured_interpretation`, `variant_product` (3); no rework or evidence-integrity failure.
|
||||
- Review closures are true; scores 2/0/2/1/2 = G07; official review filename `CODE_REVIEW-cloud-G07.md`.
|
||||
|
||||
## Dependencies and Execution Order
|
||||
|
||||
1. Require `04+02_preset_refresh` completion.
|
||||
2. Add and validate the config DTOs.
|
||||
3. Preserve the catalog in `NodeStore`, classify any catalog mutation as restart-required, then synchronize docs.
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Define and fail-closed validate the globally unique operator workspace catalog, closed operations, fixed command templates, Mac platform, and numeric/environment boundaries.
|
||||
- [ ] Preserve immutable workspace capabilities in `NodeStore`, expose exact-ref lookup, and classify workspace changes as restart-required.
|
||||
- [ ] Synchronize the config example, inner config contract, and provider/config-refresh living spec without claiming runtime execution.
|
||||
- [ ] Run dependency, focused race, package, vet, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Implementation Plan
|
||||
|
||||
### [API-1] Add the approved workspace catalog schema
|
||||
|
||||
**Problem**
|
||||
|
||||
- `packages/go/config/edge_types.go:134` gives a Node only adapters, providers, and runtime metadata.
|
||||
- `packages/go/config/load.go:123` validates providers but has no workspace uniqueness or capability validation.
|
||||
|
||||
**Solution**
|
||||
|
||||
Before (`packages/go/config/edge_types.go:134`):
|
||||
|
||||
```go
|
||||
type NodeDefinition struct {
|
||||
ID string
|
||||
Alias string
|
||||
Token string
|
||||
Adapters AdaptersConf
|
||||
Providers []NodeProviderConf
|
||||
Runtime RuntimeConf
|
||||
}
|
||||
```
|
||||
|
||||
After, with complete `mapstructure`/`yaml` tags:
|
||||
|
||||
```go
|
||||
type NodeDefinition struct {
|
||||
// Existing fields remain.
|
||||
Workspaces []WorkspaceDefinition
|
||||
}
|
||||
|
||||
type WorkspaceDefinition struct {
|
||||
Ref, Platform, Root string
|
||||
Operations []WorkspaceOperation
|
||||
Commands []WorkspaceCommandDefinition
|
||||
EnvironmentAllowlist []string
|
||||
MaxReadBytes, MaxWriteBytes, MaxOutputBytes, MaxCommandTimeoutMS int
|
||||
}
|
||||
```
|
||||
|
||||
Define the closed operation constants `read`, `list`, `write`, `delete`, and `command`. A command definition is an operator-owned template (`id`, absolute clean `executable`, fixed `args`); the caller/model selects only its id. Validate trimmed globally unique refs, `platform == "darwin"`, absolute clean roots other than `/`, non-empty unique operations/command ids, command presence iff command is enabled, positive bounded byte/time limits, and unique portable environment variable names. Do not stat Mac paths on Edge and do not put roots or command details in execution presets.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `packages/go/config/edge_types.go` — add typed workspace and command capability DTOs/constants.
|
||||
- [ ] `packages/go/config/load.go` — normalize and validate all workspaces before presets become observable.
|
||||
- [ ] `packages/go/config/workspace_config_test.go` — table-test valid decode plus duplicates, platform/root, operation, command, env, and numeric rejection.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Add `TestLoadEdgeWorkspaceCatalog` and `TestLoadEdgeWorkspaceCatalogRejectsInvalid` with temporary YAML fixtures.
|
||||
- Assert unknown/duplicate operations and globally duplicated refs fail closed; empty catalogs remain backward-compatible.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./packages/go/config -run 'TestLoadEdgeWorkspaceCatalog' -count=1`
|
||||
- Expected: valid Mac catalogs normalize exactly and every invalid boundary is rejected.
|
||||
|
||||
### [API-2] Compile catalog ownership and restart semantics
|
||||
|
||||
**Problem**
|
||||
|
||||
- `apps/edge/internal/node/store.go:12` drops workspace definitions while compiling nodes.
|
||||
- `apps/edge/internal/configrefresh/classify.go:223` compares alias/token/adapters/runtime but not workspace roots or capabilities.
|
||||
|
||||
**Solution**
|
||||
|
||||
Before (`apps/edge/internal/node/store.go:12`):
|
||||
|
||||
```go
|
||||
type NodeRecord struct {
|
||||
ID, Alias, Token string
|
||||
Adapters config.AdaptersConf
|
||||
Providers []config.NodeProviderConf
|
||||
Runtime config.RuntimeConf
|
||||
}
|
||||
```
|
||||
|
||||
After:
|
||||
|
||||
```go
|
||||
type NodeRecord struct {
|
||||
// Existing fields remain.
|
||||
Workspaces []config.WorkspaceDefinition
|
||||
}
|
||||
|
||||
func (s *NodeStore) ResolveWorkspace(ref string) (*NodeRecord, config.WorkspaceDefinition, error)
|
||||
```
|
||||
|
||||
Deep-copy slices/maps on store construction and lookup. Reject duplicate refs even when `LoadFromConfig` is called directly. Add workspaces to the config-refresh node key and report `nodes["<id>"].workspaces` as `restart_required`; active requests must never observe a root/capability mutation. Add only a commented, non-host-specific example in `configs/edge.yaml`.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/edge/internal/node/store.go` — retain immutable capabilities and resolve exactly one ref.
|
||||
- [ ] `apps/edge/internal/node/store_test.go` — cover lookup, copy isolation, missing/duplicate refs, and node ownership.
|
||||
- [ ] `apps/edge/internal/configrefresh/classify.go` — make workspace mutations restart-required.
|
||||
- [ ] `apps/edge/internal/configrefresh/workspace_classify_test.go` — assert root/capability changes cannot be applied live.
|
||||
- [ ] `configs/edge.yaml` — add a commented operator workspace example with no real local path or secret.
|
||||
- [ ] `agent-contract/inner/edge-config-runtime-refresh.md` — define schema ownership, validation, secrecy, and restart semantics.
|
||||
- [ ] `agent-spec/runtime/provider-pool-config-refresh.md` — record current catalog compilation and explicit runtime deferral.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Extend store tests and add a focused refresh test. Documentation uses those executable tests as its oracle.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/edge/internal/node ./apps/edge/internal/configrefresh -run 'Test(LoadFromConfig.*Workspace|NodeStore.*Workspace|ClassifyWorkspace)' -count=1`
|
||||
- `rg --sort path -n 'workspace_ref|workspaces|restart_required|darwin' configs/edge.yaml agent-contract/inner/edge-config-runtime-refresh.md agent-spec/runtime/provider-pool-config-refresh.md`
|
||||
- Expected: lookup is immutable and exact, all workspace changes require restart, and docs do not claim executor completion.
|
||||
|
||||
## Modified Files Summary
|
||||
|
||||
| File | Item |
|
||||
|------|------|
|
||||
| `packages/go/config/edge_types.go` | API-1 |
|
||||
| `packages/go/config/load.go` | API-1 |
|
||||
| `packages/go/config/workspace_config_test.go` | API-1 |
|
||||
| `apps/edge/internal/node/store.go` | API-2 |
|
||||
| `apps/edge/internal/node/store_test.go` | API-2 |
|
||||
| `apps/edge/internal/configrefresh/classify.go` | API-2 |
|
||||
| `apps/edge/internal/configrefresh/workspace_classify_test.go` | API-2 |
|
||||
| `configs/edge.yaml` | API-2 |
|
||||
| `agent-contract/inner/edge-config-runtime-refresh.md` | API-2 |
|
||||
| `agent-spec/runtime/provider-pool-config-refresh.md` | API-2 |
|
||||
| `agent-task/m-iop-owned-single-request-agent-execution/07+04_workspace_catalog/CODE_REVIEW-cloud-G07.md` | API-1, API-2 |
|
||||
|
||||
## Final Verification
|
||||
|
||||
1. `test -f agent-task/m-iop-owned-single-request-agent-execution/04+02_preset_refresh/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/04+02_preset_refresh/complete.log' | wc -l)" -eq 1`
|
||||
2. `go test -race ./packages/go/config -run 'TestLoadEdgeWorkspaceCatalog' -count=1`
|
||||
3. `go test -race ./apps/edge/internal/node ./apps/edge/internal/configrefresh -run 'Test(LoadFromConfig.*Workspace|NodeStore.*Workspace|ClassifyWorkspace)' -count=1`
|
||||
4. `go test ./packages/go/config ./apps/edge/internal/node ./apps/edge/internal/configrefresh -count=1`
|
||||
5. `go vet ./packages/go/config ./apps/edge/internal/node ./apps/edge/internal/configrefresh`
|
||||
6. `rg --sort path -n 'workspace_ref|workspaces|restart_required|darwin' configs/edge.yaml agent-contract/inner/edge-config-runtime-refresh.md agent-spec/runtime/provider-pool-config-refresh.md`
|
||||
7. `git diff --check`
|
||||
|
||||
Expected: exactly one predecessor completion exists; the catalog is fail-closed and immutable; refresh requires restart; all focused/package checks pass. Go test cache output is not acceptable because every test command uses `-count=1`.
|
||||
|
||||
**After completing all code changes, fill implementation-owned sections in `CODE_REVIEW-*-G??.md`.**
|
||||
|
|
@ -0,0 +1,165 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/08+03,07_workspace_admission plan=0 tag=API milestone-task=workspace-binding -->
|
||||
|
||||
# Code Review Reference - API
|
||||
|
||||
> **[IMPLEMENTING AGENT — READ FIRST] Filling in this file is the mandatory final step of implementation.**
|
||||
> The task is NOT complete until every implementation-owned section below is filled in.
|
||||
> Complete the `Implementation Checklist`; the final checklist item is mandatory before saving.
|
||||
> Fill implementation-owned sections, then stop with active files in place and report ready for review.
|
||||
> Execute the plan's selected root cause, scope, files, and dependency decisions as written. Do not choose another owner, narrow/expand the write boundary, or replace a fix with another verification attempt.
|
||||
> If implementation is blocked, record the exact blocker, attempted commands/output, and resume condition only in implementation-owned evidence fields.
|
||||
> Do not ask the user directly, present choices, call user-input tools, create control-plane stop files, or classify the next state.
|
||||
> Finalization (`Code Review Result`, log rename, `complete.log`, archive moves, `Review-Only Checklist`) is review-agent-only, even after compaction/resume.
|
||||
> Follow the ownership table at the bottom of this file for which sections you own.
|
||||
|
||||
## Overview
|
||||
|
||||
date=2026-08-06
|
||||
task=m-iop-owned-single-request-agent-execution/08+03,07_workspace_admission, plan=0, tag=API
|
||||
|
||||
## For the Review Agent
|
||||
|
||||
> **[REVIEW AGENT ONLY]** The finalization steps below are review-agent only. Implementing agents must not execute this section.
|
||||
|
||||
Compare implementation of each item against source files and verify that output in `Verification Results` matches code.
|
||||
Review completion means the following steps are finished:
|
||||
|
||||
1. Append verdict and `review_rework_count` / `evidence_integrity_failure` routing signals.
|
||||
2. Archive `CODE_REVIEW-cloud-G09.md` → `code_review_cloud_G09_0.log` and `PLAN-cloud-G08.md` → `plan_cloud_G08_0.log`.
|
||||
3. If PASS, write `complete.log` and move active task directory to `agent-task/archive/YYYY/MM/m-iop-owned-single-request-agent-execution/08+03,07_workspace_admission/`. If WARN/FAIL, fully write the next filesystem state required by the code-review skill.
|
||||
4. If PASS, preserve the first-line `milestone-task=workspace-binding` metadata in `complete.log` and report it for runtime aggregation. Roadmap state evaluation belongs to `sync-milestone-workstate`.
|
||||
5. Check applicable `Review-Only Checklist` items at the final `.log` location before reporting.
|
||||
|
||||
---
|
||||
|
||||
## Implementation Item Completion
|
||||
|
||||
| Item | Status |
|
||||
|------|---------|
|
||||
| API-1 Snapshot exact ready workspace ownership | [ ] |
|
||||
| API-2 Bind workspace before single-request execution | [ ] |
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Freeze each approved workspace ref to one configured Node id, ready connection generation, closed operation/command ids, and effective limits without raw root/template leakage.
|
||||
- [ ] Fail before executor startup on missing, foreign, pending, stale, malformed, or unsupported workspace ownership and prohibit fallback/reselection.
|
||||
- [ ] Prove admission immutability and reconnect/refresh races, then synchronize the runtime living spec.
|
||||
- [ ] Run exact dependency, focused race, package, vet, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Review-Only Checklist
|
||||
|
||||
> **[REVIEW AGENT ONLY]** This checklist is used only by the review agent. Implementing agents must not modify or check this section.
|
||||
|
||||
- [ ] Append one verdict of `PASS`, `WARN`, or `FAIL` and verified routing signals.
|
||||
- [ ] Verify verdict, dimensions, and finding classifications.
|
||||
- [ ] Archive active review and plan to the routed log names above.
|
||||
- [ ] Verify the Agent-Ops managed block in `.gitignore`.
|
||||
- [ ] If PASS, write `complete.log` and leave no active files in this directory.
|
||||
- [ ] If PASS, move this directory to `agent-task/archive/YYYY/MM/m-iop-owned-single-request-agent-execution/08+03,07_workspace_admission/`.
|
||||
- [ ] If PASS, preserve/report Milestone metadata without editing roadmap state directly.
|
||||
- [ ] Retain the active task-group parent while sibling work remains.
|
||||
- [ ] If WARN/FAIL, write the next filesystem state and do not write `complete.log`.
|
||||
|
||||
## Deviations from Plan
|
||||
|
||||
_Record any deviations from the plan and the rationale here._
|
||||
|
||||
## Key Design Decisions
|
||||
|
||||
_Record key design decisions here._
|
||||
|
||||
## Reviewer Checkpoints
|
||||
|
||||
- Confirm exact ref-to-configured-node resolution and no caller Node/path fallback.
|
||||
- Confirm the snapshot copies generation/capabilities and executor start happens only afterward.
|
||||
- Confirm reconnect and refresh cannot retarget an admitted request.
|
||||
- Confirm raw roots, executable paths, fixed args, and environment values do not enter the coordinator-facing binding.
|
||||
|
||||
## Verification Results
|
||||
|
||||
Paste actual stdout/stderr under each command; command substitutions require a recorded deviation.
|
||||
|
||||
### 1. Packet 03 dependency
|
||||
|
||||
`test -f agent-task/m-iop-owned-single-request-agent-execution/03+02_single_request_coordinator/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/03+02_single_request_coordinator/complete.log' | wc -l)" -eq 1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 2. Packet 07 dependency
|
||||
|
||||
`test -f agent-task/m-iop-owned-single-request-agent-execution/07+04_workspace_catalog/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/07+04_workspace_catalog/complete.log' | wc -l)" -eq 1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 3. Registry race test
|
||||
|
||||
`go test -race ./apps/edge/internal/node -run 'TestRegistryReadyOwnerSnapshot' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 4. Admission race test
|
||||
|
||||
`go test -race ./apps/edge/internal/service -run 'TestSingleRequestWorkspace' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 5. Package regression
|
||||
|
||||
`go test ./apps/edge/internal/node ./apps/edge/internal/service -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 6. Vet
|
||||
|
||||
`go vet ./apps/edge/internal/node ./apps/edge/internal/service`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 7. Spec search
|
||||
|
||||
`rg --sort path -n 'workspace_ref|connection generation|admission|reselect|defer' agent-spec/runtime/edge-node-execution.md`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 8. Whitespace
|
||||
|
||||
`git diff --check`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
> **[IMPLEMENTING AGENT — BEFORE SAVING] Have you filled in every implementation-owned section?**
|
||||
> If anything is blank, go back and fill it in before saving this file.
|
||||
> Leave review-agent-only sections unchanged.
|
||||
|
||||
## Section Ownership
|
||||
|
||||
| Section | Owner | Note |
|
||||
|---------|-------|------|
|
||||
| Header comment, Overview, Review Agent Instructions | Fixed at stub creation | Implementing agent must not modify or execute these (archive, complete.log, and task-directory archive move are review-agent only) |
|
||||
| Archive Evidence Snapshot | Fixed at stub creation from plan when present | Implementing agent uses it as default prior-loop context; read only the specific archive files cited there when more detail is required |
|
||||
| Implementation Item Completion (item names) | Fixed at stub creation | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Implementation Checklist (item text/order) | Fixed at stub creation from plan | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Review-Only Checklist | Review agent only | Implementing agent must not modify or check this section |
|
||||
| Deviations from Plan, Key Design Decisions | Implementing agent | Replace placeholder text with actual content |
|
||||
| Reviewer Checkpoints | Fixed at stub creation | Pre-filled from plan |
|
||||
| Verification Results (section headings + commands) | Fixed at stub creation | Implementing agent fills in command output only; command changes require a `Deviations from Plan` entry |
|
||||
| Code Review Result | Review agent appends | Not included in stub |
|
||||
|
|
@ -0,0 +1,223 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/08+03,07_workspace_admission plan=0 tag=API milestone-task=workspace-binding -->
|
||||
|
||||
# Request-stable Workspace Admission
|
||||
|
||||
## For the Implementing Agent
|
||||
|
||||
Do not start until packets 03 and 07 each have `complete.log`. Implement exactly within `Modified Files Summary`, run all verification, fill `CODE_REVIEW-cloud-G09.md`, and leave loop finalization to the official reviewer. A blocker belongs only in implementation evidence with its resume condition.
|
||||
|
||||
## Background
|
||||
|
||||
Packet 07 maps an opaque ref to one configured Node; packet 03 starts a request-local coordinator. The remaining admission gap is to freeze that ref to the exact dispatch-ready Node connection generation and effective capabilities before any provider or tool work begins.
|
||||
|
||||
## Analysis
|
||||
|
||||
### Files Read
|
||||
|
||||
- `AGENTS.md`
|
||||
- `agent-ops/rules/project/rules.md`
|
||||
- `agent-ops/rules/common/rules-roadmap.md`
|
||||
- `agent-ops/rules/common/rules-agent-spec.md`
|
||||
- `agent-ops/rules/project/domain/edge/rules.md`
|
||||
- `agent-test/local/rules.md`
|
||||
- `agent-test/local/edge-smoke.md`
|
||||
- `agent-roadmap/priority-queue.md`
|
||||
- `agent-roadmap/phase/knowledge-tool-optimization-extension/PHASE.md`
|
||||
- `agent-roadmap/phase/knowledge-tool-optimization-extension/milestones/iop-owned-single-request-agent-execution.md`
|
||||
- `agent-roadmap/sdd/knowledge-tool-optimization-extension/iop-owned-single-request-agent-execution/SDD.md`
|
||||
- `agent-task/m-iop-owned-single-request-agent-execution/02+01_preset_binding/PLAN-local-G06.md`
|
||||
- `agent-task/m-iop-owned-single-request-agent-execution/03+02_single_request_coordinator/PLAN-local-G07.md`
|
||||
- `apps/edge/internal/service/service.go`
|
||||
- `apps/edge/internal/node/store.go`
|
||||
- `apps/edge/internal/node/store_test.go`
|
||||
- `apps/edge/internal/node/registry.go`
|
||||
- `apps/edge/internal/node/registry_test.go`
|
||||
- `agent-contract/inner/edge-config-runtime-refresh.md`
|
||||
- `agent-spec/runtime/edge-node-execution.md`
|
||||
- `agent-spec/runtime/provider-pool-config-refresh.md`
|
||||
|
||||
### SDD Criteria
|
||||
|
||||
- S04 requires approved workspace admission and pre-execution rejection of unapproved ref, foreign Node/path, and escape candidates.
|
||||
- The request's workspace generation is immutable alongside planner/worker/reviewer bindings; refresh or reconnect cannot silently retarget it.
|
||||
- This packet supplies admission and generation fencing. Packet 10 supplies filesystem/symlink enforcement.
|
||||
|
||||
### Verification Context
|
||||
|
||||
- Baseline targeted packages passed fresh at the starting HEAD.
|
||||
- Both predecessor APIs are intentionally unavailable in current source; their active plans define `SingleRequestBinding`, `StartSingleRequest`, and the catalog contract. Implementation must first verify their exact `complete.log` evidence and then use the implemented symbols without changing predecessor ownership.
|
||||
- Deterministic evidence is race-tested service/node unit tests; no Mac runner is needed for admission-only behavior.
|
||||
|
||||
### State and Concurrency Findings
|
||||
|
||||
- A ready Node connection owns `(node_id, connection_generation)`; reconnect creates a strictly larger generation.
|
||||
- Admission must copy capability ids/limits, never retain mutable config slices, and must not store the raw root or command executable/args in the coordinator-facing binding.
|
||||
- Unavailable, pending, stale, foreign, or ambiguous ownership fails before the executor starts; no implicit single-node fallback is allowed for `workspace_ref`.
|
||||
|
||||
### Test Coverage Gaps
|
||||
|
||||
- No test binds a workspace ref to a ready generation or proves reconnect/refresh isolation and executor non-invocation on rejection.
|
||||
|
||||
### Symbol References
|
||||
|
||||
- Packet 02 plans `SingleRequestBinding.WorkspaceRef`; extend rather than replace its public model/stage/limit fields.
|
||||
- Packet 03 plans `Service.StartSingleRequest`; workspace binding must wrap its admission before the executor call.
|
||||
- Additive registry snapshot helpers must not change existing `ResolveReady` behavior.
|
||||
|
||||
### Split Judgment
|
||||
|
||||
- Stable contract: a request-stable `SingleRequestWorkspaceBinding` can be independently tested with a fake executor and no wire.
|
||||
- Wire dispatch remains packet 09 because mixing transport would prevent admission-only PASS evidence.
|
||||
|
||||
### Scope Rationale
|
||||
|
||||
- Include exact ref lookup, ready-generation snapshot, effective capability copy, start-time rejection, and living-spec sync.
|
||||
- Exclude protobuf, Node root validation, actual tool execution, tool loop, and cleanup.
|
||||
|
||||
### Final Routing
|
||||
|
||||
- `evaluation_mode=first-pass`; build closures true, scores 2/2/1/1/2 = G08.
|
||||
- Finalizer route is `risk-boundary`, lane `cloud`, filename `PLAN-cloud-G08.md`; risks are `temporal_state`, `concurrent_consistency`, `boundary_contract`, and `variant_product` (4).
|
||||
- Review scores 2/2/1/2/2 = G09; official review filename `CODE_REVIEW-cloud-G09.md`. No rework/evidence-integrity signal and no capability gap.
|
||||
|
||||
## Dependencies and Execution Order
|
||||
|
||||
1. `03+02_single_request_coordinator` must complete.
|
||||
2. `07+04_workspace_catalog` must complete.
|
||||
3. Add a cloned ready-owner snapshot, compile the binding, then insert it before executor startup.
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Freeze each approved workspace ref to one configured Node id, ready connection generation, closed operation/command ids, and effective limits without raw root/template leakage.
|
||||
- [ ] Fail before executor startup on missing, foreign, pending, stale, malformed, or unsupported workspace ownership and prohibit fallback/reselection.
|
||||
- [ ] Prove admission immutability and reconnect/refresh races, then synchronize the runtime living spec.
|
||||
- [ ] Run exact dependency, focused race, package, vet, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Implementation Plan
|
||||
|
||||
### [API-1] Snapshot exact ready workspace ownership
|
||||
|
||||
**Problem**
|
||||
|
||||
- `apps/edge/internal/node/registry.go:325` returns a shared ready entry and callers can otherwise re-resolve a different connection after admission.
|
||||
- Packet 07's catalog lookup identifies the configured Node but not its live generation.
|
||||
|
||||
**Solution**
|
||||
|
||||
Before (`apps/edge/internal/node/registry.go:325`):
|
||||
|
||||
```go
|
||||
func (r *Registry) GetReady(nodeID string) (*NodeEntry, bool) {
|
||||
r.mu.RLock()
|
||||
defer r.mu.RUnlock()
|
||||
e, ok := r.byID[nodeID]
|
||||
if !ok || !e.DispatchReady {
|
||||
return nil, false
|
||||
}
|
||||
return e, true
|
||||
}
|
||||
```
|
||||
|
||||
After, preserve existing behavior and add a cloned API:
|
||||
|
||||
```go
|
||||
func (r *Registry) ReadyOwnerSnapshot(nodeID string) (*NodeEntry, bool) {
|
||||
// Return Clone() only when DispatchReady under the same read lock.
|
||||
}
|
||||
```
|
||||
|
||||
Use packet 07's exact ref lookup to derive the configured Node id. Never accept caller Node/path input and never use `ResolveReady("")` fallback.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/edge/internal/node/registry.go` — add an atomic cloned ready-owner snapshot.
|
||||
- [ ] `apps/edge/internal/node/registry_test.go` — cover pending, ready, disconnect/reconnect generation, and copy isolation.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Add `TestRegistryReadyOwnerSnapshot` and a reconnect race case under `-race`.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/edge/internal/node -run 'TestRegistryReadyOwnerSnapshot' -count=1`
|
||||
- Expected: snapshots are immutable and never represent a pending or superseded connection.
|
||||
|
||||
### [API-2] Bind workspace before single-request execution
|
||||
|
||||
**Problem**
|
||||
|
||||
- Packet 02's planned `SingleRequestBinding` carries only opaque `WorkspaceRef` (`02+01_preset_binding/PLAN-local-G06.md:120`).
|
||||
- Packet 03's planned `StartSingleRequest` snapshots the executor and immediately calls it (`03+02_single_request_coordinator/PLAN-local-G07.md:140`), leaving no concrete workspace admission.
|
||||
|
||||
**Solution**
|
||||
|
||||
Before (predecessor contract, `03+02_single_request_coordinator/PLAN-local-G07.md:140`):
|
||||
|
||||
```go
|
||||
func (s *Service) StartSingleRequest(ctx context.Context, req SingleRequestRequest) (SingleRequestExecution, error) {
|
||||
s.mu.RLock()
|
||||
executor := s.singleRequestExecutor
|
||||
s.mu.RUnlock()
|
||||
return startSingleRequest(ctx, executor, req)
|
||||
}
|
||||
```
|
||||
|
||||
After:
|
||||
|
||||
```go
|
||||
bound, err := s.bindSingleRequestWorkspace(req.Binding)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
req.Binding = bound
|
||||
return startSingleRequest(ctx, executor, req)
|
||||
```
|
||||
|
||||
Add `SingleRequestWorkspaceBinding` with only `Ref`, `NodeID`, `ConnectionGeneration`, cloned operation/command ids, and workspace/preset effective maxima. Keep raw root, executable, fixed args, and environment values out. Validate the original binding and catalog agree; use the lower applicable preset/workspace bound. Recheck generation immediately before executor handoff and make the executor receive only the frozen value.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/edge/internal/service/single_request_types.go` — extend the predecessor DTO with a frozen workspace binding and deep-copy validation.
|
||||
- [ ] `apps/edge/internal/service/single_request.go` — bind before executor startup and fail closed without emitting a stage.
|
||||
- [ ] `apps/edge/internal/service/single_request_workspace.go` — own exact catalog/registry admission and effective-limit calculation.
|
||||
- [ ] `apps/edge/internal/service/single_request_workspace_test.go` — cover approval matrix, no fallback, pending/stale/reconnect, refresh mutation, copy isolation, and executor non-invocation.
|
||||
- [ ] `agent-spec/runtime/edge-node-execution.md` — document request-generation workspace admission and defer executor/wire claims.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Use a recording executor and real `NodeStore`/`Registry`. Mutate source config after admission and reconnect the same Node id to prove the frozen generation does not retarget.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/edge/internal/service -run 'TestSingleRequestWorkspace' -count=1`
|
||||
- `rg --sort path -n 'workspace_ref|connection generation|admission|reselect|defer' agent-spec/runtime/edge-node-execution.md`
|
||||
- Expected: only one approved ready generation reaches the executor and rejected paths emit no provider/tool work.
|
||||
|
||||
## Modified Files Summary
|
||||
|
||||
| File | Item |
|
||||
|------|------|
|
||||
| `apps/edge/internal/node/registry.go` | API-1 |
|
||||
| `apps/edge/internal/node/registry_test.go` | API-1 |
|
||||
| `apps/edge/internal/service/single_request_types.go` | API-2 |
|
||||
| `apps/edge/internal/service/single_request.go` | API-2 |
|
||||
| `apps/edge/internal/service/single_request_workspace.go` | API-2 |
|
||||
| `apps/edge/internal/service/single_request_workspace_test.go` | API-2 |
|
||||
| `agent-spec/runtime/edge-node-execution.md` | API-2 |
|
||||
| `agent-task/m-iop-owned-single-request-agent-execution/08+03,07_workspace_admission/CODE_REVIEW-cloud-G09.md` | API-1, API-2 |
|
||||
|
||||
## Final Verification
|
||||
|
||||
1. `test -f agent-task/m-iop-owned-single-request-agent-execution/03+02_single_request_coordinator/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/03+02_single_request_coordinator/complete.log' | wc -l)" -eq 1`
|
||||
2. `test -f agent-task/m-iop-owned-single-request-agent-execution/07+04_workspace_catalog/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/07+04_workspace_catalog/complete.log' | wc -l)" -eq 1`
|
||||
3. `go test -race ./apps/edge/internal/node -run 'TestRegistryReadyOwnerSnapshot' -count=1`
|
||||
4. `go test -race ./apps/edge/internal/service -run 'TestSingleRequestWorkspace' -count=1`
|
||||
5. `go test ./apps/edge/internal/node ./apps/edge/internal/service -count=1`
|
||||
6. `go vet ./apps/edge/internal/node ./apps/edge/internal/service`
|
||||
7. `rg --sort path -n 'workspace_ref|connection generation|admission|reselect|defer' agent-spec/runtime/edge-node-execution.md`
|
||||
8. `git diff --check`
|
||||
|
||||
Expected: both predecessors resolve exactly once; ready-owner and request bindings remain immutable under races; rejection precedes executor activity; all package checks pass. Cached Go tests are not acceptable.
|
||||
|
||||
**After completing all code changes, fill implementation-owned sections in `CODE_REVIEW-*-G??.md`.**
|
||||
|
|
@ -0,0 +1,194 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/09+08_workspace_wire plan=1 tag=API milestone-task=tool-executor -->
|
||||
|
||||
# Code Review Reference - API
|
||||
|
||||
> **[IMPLEMENTING AGENT — READ FIRST] Filling in this file is the mandatory final step of implementation.**
|
||||
> The task is NOT complete until every implementation-owned section below is filled in.
|
||||
> Complete the `Implementation Checklist`; the final checklist item is mandatory before saving.
|
||||
> Fill implementation-owned sections, then stop with active files in place and report ready for review.
|
||||
> Execute the plan's selected root cause, scope, files, and dependency decisions as written. Do not choose another owner, narrow/expand the write boundary, or replace a fix with another verification attempt.
|
||||
> If implementation is blocked, record the exact blocker, attempted commands/output, and resume condition only in implementation-owned evidence fields.
|
||||
> Do not ask the user directly, present choices, call user-input tools, create control-plane stop files, or classify the next state.
|
||||
> Finalization (`Code Review Result`, log rename, `complete.log`, archive moves, `Review-Only Checklist`) is review-agent-only, even after compaction/resume.
|
||||
> Follow the ownership table at the bottom of this file for which sections you own.
|
||||
|
||||
## Overview
|
||||
|
||||
date=2026-08-06
|
||||
task=m-iop-owned-single-request-agent-execution/09+08_workspace_wire, plan=1, tag=API
|
||||
|
||||
## Archive Evidence Snapshot
|
||||
|
||||
- The first-pass pair is preserved at `agent-task/m-iop-owned-single-request-agent-execution/09+08_workspace_wire/plan_cloud_G08_0.log` and `agent-task/m-iop-owned-single-request-agent-execution/09+08_workspace_wire/code_review_cloud_G09_0.log`; it contains no implementation evidence or review verdict.
|
||||
- Self-review found that the wire identity must use the immutable coordinator `request_id`, and that changing `runtime.proto` requires tracked Go and Dart regeneration plus client verification. Plan 1 is the isolated reassessment that corrects both omissions.
|
||||
|
||||
## For the Review Agent
|
||||
|
||||
> **[REVIEW AGENT ONLY]** The finalization steps below are review-agent only. Implementing agents must not execute this section.
|
||||
|
||||
Compare implementation of each item against source files and verify that output in `Verification Results` matches code.
|
||||
Review completion means the following steps are finished:
|
||||
|
||||
1. Append verdict and `review_rework_count` / `evidence_integrity_failure` routing signals.
|
||||
2. Archive `CODE_REVIEW-cloud-G09.md` → `code_review_cloud_G09_1.log` and `PLAN-cloud-G08.md` → `plan_cloud_G08_1.log`.
|
||||
3. If PASS, write `complete.log` and move active task directory to `agent-task/archive/YYYY/MM/m-iop-owned-single-request-agent-execution/09+08_workspace_wire/`. If WARN/FAIL, fully write the next filesystem state required by the code-review skill.
|
||||
4. If PASS, preserve the first-line `milestone-task=tool-executor` metadata in `complete.log` and report it for runtime aggregation. Roadmap state evaluation belongs to `sync-milestone-workstate`.
|
||||
5. Check applicable `Review-Only Checklist` items at the final `.log` location before reporting.
|
||||
|
||||
---
|
||||
|
||||
## Implementation Item Completion
|
||||
|
||||
| Item | Status |
|
||||
|------|---------|
|
||||
| API-1 Define the workspace protocol and catalog payload | [ ] |
|
||||
| API-2 Register compatible parsers and optional Node handlers | [ ] |
|
||||
| API-3 Dispatch only to the admitted generation | [ ] |
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Define and generate a dedicated typed workspace config/open/tool/cancel/cleanup protocol with closed operations, immutable `request_id`/stage/tool identities, statuses, error codes, and bounded result fields.
|
||||
- [ ] Deliver approved capabilities in `NodeConfigPayload` and register backward-compatible Edge/Node parsers plus an optional Node workspace handler.
|
||||
- [ ] Implement a generation-fenced service wire client that never reselects a Node and propagates timeout/context cancellation without raw logging.
|
||||
- [ ] Prove Go and Dart generation cleanliness, parser/round-trip/cancel/stale-generation behavior, and synchronize the wire contract/living spec.
|
||||
- [ ] Run dependency, Go/Dart generation, focused race, package, vet, client test/build, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Review-Only Checklist
|
||||
|
||||
> **[REVIEW AGENT ONLY]** Implementing agents must not modify or check this section.
|
||||
|
||||
- [ ] Append PASS/WARN/FAIL with verified routing signals and matching findings/dimensions.
|
||||
- [ ] Archive the active pair to the routed `*_1.log` names.
|
||||
- [ ] Verify the managed `.gitignore` block.
|
||||
- [ ] On PASS write `complete.log`, preserve/report Milestone metadata, and move this directory to the monthly archive.
|
||||
- [ ] Keep the active task-group parent while siblings remain.
|
||||
- [ ] On WARN/FAIL write only the code-review skill's required next state.
|
||||
|
||||
## Deviations from Plan
|
||||
|
||||
_Record deviations and rationale here._
|
||||
|
||||
## Key Design Decisions
|
||||
|
||||
_Record key implemented decisions here._
|
||||
|
||||
## Reviewer Checkpoints
|
||||
|
||||
- Confirm no workspace data was added to `RunRequest`, provider execution, or `NodeCommand`.
|
||||
- Confirm protobuf field numbering, oneof use, closed enums, bounds, immutable `request_id`, and both Go/Dart generated-file provenance.
|
||||
- Confirm existing `Handler` mocks remain source-compatible through an optional interface.
|
||||
- Confirm every dispatch checks admitted Node id/generation and never reselects after reconnect.
|
||||
- Confirm logs/errors do not expose raw tool inputs/results or config secrets.
|
||||
|
||||
## Verification Results
|
||||
|
||||
Paste actual stdout/stderr for every command; record any replacement under deviations.
|
||||
|
||||
### 1. Dependency
|
||||
|
||||
`test -f agent-task/m-iop-owned-single-request-agent-execution/08+03,07_workspace_admission/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/08+03,07_workspace_admission/complete.log' | wc -l)" -eq 1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 2. Protobuf generation
|
||||
|
||||
`make proto`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 3. Dart protobuf generation
|
||||
|
||||
`make proto-dart`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 4. Generated-file scope
|
||||
|
||||
`git diff --exit-code -- proto/gen/iop/agent.pb.go proto/gen/iop/control.pb.go proto/gen/iop/job.pb.go proto/gen/iop/node.pb.go apps/client/lib/gen/proto/iop/{control,job,node}.{pb,pbenum,pbjson,pbserver}.dart`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 5. Focused race tests
|
||||
|
||||
`go test -race ./apps/edge/internal/node ./apps/edge/internal/service ./apps/edge/internal/transport ./apps/node/internal/transport -run 'Test(BuildConfigPayload.*Workspace|WorkspaceWire|NodeParserMapWorkspace|SessionWorkspace|EdgeParserMapWorkspace)' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 6. Package regression
|
||||
|
||||
`go test ./apps/edge/internal/node ./apps/edge/internal/service ./apps/edge/internal/transport ./apps/node/internal/transport -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 7. Vet
|
||||
|
||||
`go vet ./apps/edge/internal/node ./apps/edge/internal/service ./apps/edge/internal/transport ./apps/node/internal/transport`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 8. Client tests
|
||||
|
||||
`make client-test`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 9. Client web build
|
||||
|
||||
`make client-build-web`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 10. Contract/spec search
|
||||
|
||||
`rg --sort path -n 'Workspace(Open|Tool|Cancel|Cleanup)|request_id|RunRequest|NodeCommand|generation|raw' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 11. Whitespace
|
||||
|
||||
`git diff --check`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
> **[IMPLEMENTING AGENT — BEFORE SAVING] Have you filled in every implementation-owned section?**
|
||||
> If anything is blank, go back and fill it in before saving this file.
|
||||
> Leave review-agent-only sections unchanged.
|
||||
|
||||
## Section Ownership
|
||||
|
||||
| Section | Owner | Note |
|
||||
|---------|-------|------|
|
||||
| Header comment, Overview, Review Agent Instructions | Fixed at stub creation | Implementing agent must not modify or execute these (archive, complete.log, and task-directory archive move are review-agent only) |
|
||||
| Archive Evidence Snapshot | Fixed at stub creation from plan when present | Implementing agent uses it as default prior-loop context; read only the specific archive files cited there when more detail is required |
|
||||
| Implementation Item Completion (item names) | Fixed at stub creation | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Implementation Checklist (item text/order) | Fixed at stub creation from plan | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Review-Only Checklist | Review agent only | Implementing agent must not modify or check this section |
|
||||
| Deviations from Plan, Key Design Decisions | Implementing agent | Replace placeholder text with actual content |
|
||||
| Reviewer Checkpoints | Fixed at stub creation | Pre-filled from plan |
|
||||
| Verification Results (section headings + commands) | Fixed at stub creation | Implementing agent fills in command output only; command changes require a `Deviations from Plan` entry |
|
||||
| Code Review Result | Review agent appends | Not included in stub |
|
||||
|
|
@ -0,0 +1,296 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/09+08_workspace_wire plan=1 tag=API milestone-task=tool-executor -->
|
||||
|
||||
# Dedicated Edge-Node Workspace Runtime Wire
|
||||
|
||||
## For the Implementing Agent
|
||||
|
||||
Do not start until packet 08 has `complete.log`. Keep all changes inside the listed boundary, regenerate both Go and Dart bindings only with the repository targets, run every command, fill the paired review stub, and leave review/finalization artifacts to the official reviewer.
|
||||
|
||||
## Background
|
||||
|
||||
The admitted binding needs a transport that is distinct from provider `RunRequest`, provider execution, and closed `NodeCommand`. This packet defines and proves that typed boundary, including catalog delivery, open/tool/cancel/cleanup requests, generation fencing, and bounded replies; Node execution remains unsupported until packet 10.
|
||||
|
||||
## Archive Evidence Snapshot
|
||||
|
||||
- The first-pass pair is preserved at `agent-task/m-iop-owned-single-request-agent-execution/09+08_workspace_wire/plan_cloud_G08_0.log` and `agent-task/m-iop-owned-single-request-agent-execution/09+08_workspace_wire/code_review_cloud_G09_0.log`; it contains no implementation evidence or review verdict.
|
||||
- Self-review found that the wire identity must use the immutable coordinator `request_id`, and that changing `runtime.proto` requires tracked Go and Dart regeneration plus client verification. Plan 1 is the isolated reassessment that corrects both omissions.
|
||||
|
||||
## Analysis
|
||||
|
||||
### Files Read
|
||||
|
||||
- `AGENTS.md`
|
||||
- `agent-ops/rules/project/rules.md`
|
||||
- `agent-ops/rules/common/rules-roadmap.md`
|
||||
- `agent-ops/rules/common/rules-agent-spec.md`
|
||||
- `agent-ops/rules/project/domain/edge/rules.md`
|
||||
- `agent-ops/rules/project/domain/node/rules.md`
|
||||
- `agent-ops/rules/project/domain/platform-common/rules.md`
|
||||
- `agent-ops/rules/project/domain/client/rules.md`
|
||||
- `agent-test/local/rules.md`
|
||||
- `agent-test/local/edge-smoke.md`
|
||||
- `agent-test/local/node-smoke.md`
|
||||
- `agent-test/local/platform-common-smoke.md`
|
||||
- `agent-test/local/client-smoke.md`
|
||||
- `agent-roadmap/phase/knowledge-tool-optimization-extension/milestones/iop-owned-single-request-agent-execution.md`
|
||||
- `agent-roadmap/sdd/knowledge-tool-optimization-extension/iop-owned-single-request-agent-execution/SDD.md`
|
||||
- `proto/iop/runtime.proto`
|
||||
- `Makefile`
|
||||
- `apps/client/lib/gen/proto/iop/runtime.pb.dart`
|
||||
- `apps/client/lib/gen/proto/iop/runtime.pbenum.dart`
|
||||
- `apps/client/lib/gen/proto/iop/runtime.pbjson.dart`
|
||||
- `apps/client/lib/gen/proto/iop/runtime.pbserver.dart`
|
||||
- `apps/edge/internal/node/mapper.go`
|
||||
- `apps/edge/internal/node/mapper_test.go`
|
||||
- `apps/edge/internal/service/service.go`
|
||||
- `apps/edge/internal/service/node_command.go`
|
||||
- `apps/edge/internal/node/registry.go`
|
||||
- `apps/edge/internal/transport/server.go`
|
||||
- `apps/edge/internal/transport/server_test.go`
|
||||
- `apps/node/internal/transport/parser.go`
|
||||
- `apps/node/internal/transport/parser_test.go`
|
||||
- `apps/node/internal/transport/session.go`
|
||||
- `apps/node/internal/transport/session_test.go`
|
||||
- `agent-contract/inner/edge-node-runtime-wire.md`
|
||||
- `agent-contract/inner/execution-runtime.md`
|
||||
- `agent-spec/runtime/edge-node-execution.md`
|
||||
|
||||
### SDD Criteria
|
||||
|
||||
- D08 and S05 require a separate typed workspace request/result boundary, never `RunRequest.metadata`, provider execution, or `NodeCommand` extension.
|
||||
- Requests/results must cover success/error/timeout/large output and process cancellation with immutable coordinator request/stage/tool identity. The wire `request_id` is the identity later used for `.iop/job/<request_id>`; it is not a Node-local execution id.
|
||||
- S07 requires cleanup to be an explicit request-owned action; execution semantics follow in packet 13.
|
||||
|
||||
### Verification Context
|
||||
|
||||
- `make -n proto` and `make -n proto-dart` resolve to repository-owned Go and Dart generation; `protoc`, `protoc-gen-dart`, Flutter, and Dart are installed.
|
||||
- Baseline Edge/Node transport and service packages passed fresh.
|
||||
- Packet 08 supplies the exact admitted Node id/generation DTO. Wire tests can use `net.Pipe` and do not require a Mac filesystem.
|
||||
|
||||
### State and Concurrency Findings
|
||||
|
||||
- Each send must use the admitted Node id and generation; reconnect must fail as stale, not re-resolve.
|
||||
- Context cancellation sends typed cancel and leaves any request waiter bounded by its timeout.
|
||||
- Transport error messages/log fields must not include path, content, argv/template, environment values, stdout/stderr, or credentials.
|
||||
|
||||
### Test Coverage Gaps
|
||||
|
||||
- Parser maps and Session have no workspace message types or optional workspace handler.
|
||||
- `NodeConfigPayload` cannot carry the approved catalog and Edge has no generation-fenced typed client.
|
||||
|
||||
### Symbol References
|
||||
|
||||
- Keep `transport.Handler` source-compatible for all existing mocks by adding a separate optional `WorkspaceHandler` interface and type assertion.
|
||||
- `RunRequest` reserved workspace fields and `NodeCommand` enum remain untouched.
|
||||
- Tracked generated output for `runtime.proto` is `proto/gen/iop/runtime.pb.go` plus the four `apps/client/lib/gen/proto/iop/runtime.*.dart` bindings.
|
||||
|
||||
### Split Judgment
|
||||
|
||||
- Stable contract: proto generation, parser registration, optional handler behavior, and a net-pipe round trip independently PASS before filesystem effects.
|
||||
- Executor behavior remains packet 10 to keep wire verification deterministic and host-neutral.
|
||||
|
||||
### Scope Rationale
|
||||
|
||||
- Include proto/config payload, Edge client, parser/listener registration, tests, contract, and spec.
|
||||
- Exclude filesystem/process implementation, stage-provider decoding, cleanup effects, and public API output.
|
||||
|
||||
### Final Routing
|
||||
|
||||
- `evaluation_mode=isolated-reassessment`; `review_rework_count=0`; `evidence_integrity_failure=false`; build closures true, scores 2/1/2/1/2 = G08.
|
||||
- Finalizer route `risk-boundary`, lane `cloud`, filename `PLAN-cloud-G08.md`; positive risks are all five routing signatures.
|
||||
- Review scores 2/1/2/2/2 = G09; official review filename `CODE_REVIEW-cloud-G09.md`. No capability gap, rework, or evidence-integrity failure.
|
||||
|
||||
## Dependencies and Execution Order
|
||||
|
||||
1. Require packet 08 completion.
|
||||
2. Add source proto messages and regenerate tracked Go and Dart bindings.
|
||||
3. Map workspace catalog into registration payload, then register parsers/listeners.
|
||||
4. Add the generation-fenced Edge client and round-trip tests before docs.
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Define and generate a dedicated typed workspace config/open/tool/cancel/cleanup protocol with closed operations, immutable `request_id`/stage/tool identities, statuses, error codes, and bounded result fields.
|
||||
- [ ] Deliver approved capabilities in `NodeConfigPayload` and register backward-compatible Edge/Node parsers plus an optional Node workspace handler.
|
||||
- [ ] Implement a generation-fenced service wire client that never reselects a Node and propagates timeout/context cancellation without raw logging.
|
||||
- [ ] Prove Go and Dart generation cleanliness, parser/round-trip/cancel/stale-generation behavior, and synchronize the wire contract/living spec.
|
||||
- [ ] Run dependency, Go/Dart generation, focused race, package, vet, client test/build, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Implementation Plan
|
||||
|
||||
### [API-1] Define the workspace protocol and catalog payload
|
||||
|
||||
**Problem**
|
||||
|
||||
- `proto/iop/runtime.proto:298` carries only adapters/runtime.
|
||||
- `proto/iop/runtime.proto:10` explicitly reserves legacy workspace fields on `RunRequest`, and `NodeCommandRequest` is a closed ops surface.
|
||||
|
||||
**Solution**
|
||||
|
||||
Before (`proto/iop/runtime.proto:298`):
|
||||
|
||||
```proto
|
||||
message NodeConfigPayload {
|
||||
repeated AdapterConfig adapters = 1;
|
||||
NodeRuntimeConfig runtime = 2;
|
||||
}
|
||||
```
|
||||
|
||||
After, add a new field and separate top-level protocol messages:
|
||||
|
||||
```proto
|
||||
message NodeConfigPayload {
|
||||
repeated AdapterConfig adapters = 1;
|
||||
NodeRuntimeConfig runtime = 2;
|
||||
repeated WorkspaceConfig workspaces = 3;
|
||||
}
|
||||
|
||||
message WorkspaceOpenRequest { /* request_id, workspace_ref, limits */ }
|
||||
message WorkspaceToolRequest { /* request/stage/tool ids, closed operation, typed input */ }
|
||||
message WorkspaceCancelRequest { /* request_id, tool_call_id */ }
|
||||
message WorkspaceCleanupRequest { /* request_id */ }
|
||||
```
|
||||
|
||||
`WorkspaceConfig` carries platform/root, closed operations, fixed command definitions, env names, and hard caps from packet 07. Tool input uses a proto `oneof` for relative path, write content, or command id/environment map. The immutable coordinator `request_id` is copied unchanged through open/tool/cancel/cleanup and later names the exact internal namespace `.iop/job/<request_id>`; do not introduce a second `execution_id` alias. Responses echo request/stage/tool identities and use closed status/error-code enums with typed bounded content/list/stdout/stderr, exit code, truncation, duration, and cleanup counts. Reserve no caller-selected Node/root/executable/argv field. Preserve existing field numbers and never reuse reservations.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `proto/iop/runtime.proto` — define catalog and four request/response families.
|
||||
- [ ] `proto/gen/iop/runtime.pb.go` — regenerate with `make proto`; no hand edits.
|
||||
- [ ] `apps/client/lib/gen/proto/iop/runtime.pb.dart` — regenerate with `make proto-dart`; no hand edits.
|
||||
- [ ] `apps/client/lib/gen/proto/iop/runtime.pbenum.dart` — regenerate with `make proto-dart`; no hand edits.
|
||||
- [ ] `apps/client/lib/gen/proto/iop/runtime.pbjson.dart` — regenerate with `make proto-dart`; no hand edits.
|
||||
- [ ] `apps/client/lib/gen/proto/iop/runtime.pbserver.dart` — regenerate with `make proto-dart`; no hand edits.
|
||||
- [ ] `apps/edge/internal/node/mapper.go` — serialize packet 07 workspace definitions into the private Node payload.
|
||||
- [ ] `apps/edge/internal/node/mapper_test.go` — assert complete typed mapping and no legacy settings leakage.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Extend mapper tests and rely on parser round trips in API-2 for every new proto family.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `make proto`
|
||||
- `make proto-dart`
|
||||
- `git diff --exit-code -- proto/gen/iop/agent.pb.go proto/gen/iop/control.pb.go proto/gen/iop/job.pb.go proto/gen/iop/node.pb.go apps/client/lib/gen/proto/iop/{control,job,node}.{pb,pbenum,pbjson,pbserver}.dart`
|
||||
- `go test ./apps/edge/internal/node -run 'TestBuildConfigPayload.*Workspace' -count=1`
|
||||
- Expected: only `runtime.pb.go` changes and the payload retains the complete approved catalog.
|
||||
|
||||
### [API-2] Register compatible parsers and optional Node handlers
|
||||
|
||||
**Problem**
|
||||
|
||||
- `apps/node/internal/transport/session.go:17` requires every handler to implement the provider methods, so adding workspace methods there would break all mocks.
|
||||
- `apps/edge/internal/transport/server.go:35` and the Node parser map do not decode new request/response types.
|
||||
|
||||
**Solution**
|
||||
|
||||
Before (`apps/node/internal/transport/session.go:17`):
|
||||
|
||||
```go
|
||||
type Handler interface {
|
||||
OnRunRequest(context.Context, *Session, *iop.RunRequest) error
|
||||
// Existing provider methods.
|
||||
}
|
||||
```
|
||||
|
||||
After, leave `Handler` unchanged and add:
|
||||
|
||||
```go
|
||||
type WorkspaceHandler interface {
|
||||
OnWorkspaceOpen(context.Context, *Session, *iop.WorkspaceOpenRequest) (*iop.WorkspaceOpenResponse, error)
|
||||
OnWorkspaceTool(context.Context, *Session, *iop.WorkspaceToolRequest) (*iop.WorkspaceToolResponse, error)
|
||||
OnWorkspaceCancel(context.Context, *Session, *iop.WorkspaceCancelRequest) (*iop.WorkspaceCancelResponse, error)
|
||||
OnWorkspaceCleanup(context.Context, *Session, *iop.WorkspaceCleanupRequest) (*iop.WorkspaceCleanupResponse, error)
|
||||
}
|
||||
```
|
||||
|
||||
Register request listeners that type-assert `WorkspaceHandler` and return a typed unsupported/not-ready response when absent or on handler error. Register all request parsers on Node and response parsers on Edge. Do not log raw request/response fields.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/node/internal/transport/parser.go` — parse workspace requests.
|
||||
- [ ] `apps/node/internal/transport/parser_test.go` — round-trip every request shape.
|
||||
- [ ] `apps/node/internal/transport/session.go` — add optional handler/listeners and typed failure translation.
|
||||
- [ ] `apps/node/internal/transport/session_test.go` — net-pipe success, absent handler, error, and identity echo cases.
|
||||
- [ ] `apps/edge/internal/transport/server.go` — parse workspace responses.
|
||||
- [ ] `apps/edge/internal/transport/server_test.go` — round-trip every response shape.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Named tests `TestNodeParserMapWorkspace`, `TestSessionWorkspaceRequest`, and `TestEdgeParserMapWorkspace` cover all message families and preserve existing handler compile compatibility.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/node/internal/transport ./apps/edge/internal/transport -run 'Test(NodeParserMapWorkspace|SessionWorkspace|EdgeParserMapWorkspace)' -count=1`
|
||||
- Expected: typed round trips succeed; absent handlers return typed failure without panic or raw leakage.
|
||||
|
||||
### [API-3] Dispatch only to the admitted generation
|
||||
|
||||
**Problem**
|
||||
|
||||
- `apps/edge/internal/service/node_command.go:122` resolves a Node per call and has no request-stable workspace generation.
|
||||
- Context cancellation has no workspace-specific cancel/cleanup path.
|
||||
|
||||
**Solution**
|
||||
|
||||
Add `workspace_wire.go` in service with `workspaceOpen`, `workspaceTool`, `workspaceCancel`, and `workspaceCleanup` methods. Each method accepts packet 08's frozen binding, obtains `ReadyOwnerSnapshot(binding.NodeID)`, compares `ConnectionGeneration`, and sends to that exact client only. Tool/open wait uses the lower admitted deadline; a cancelled context sends typed cancel once and all goroutines remain bounded by transport timeout. Translate transport/stale/typed Node errors to stable internal errors without including raw payload.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/edge/internal/service/workspace_wire.go` — implement exact-generation typed request/response dispatch.
|
||||
- [ ] `apps/edge/internal/service/workspace_wire_test.go` — net-pipe open/tool/cancel/cleanup, timeout, cancellation, stale generation, and no-reselection tests.
|
||||
- [ ] `agent-contract/inner/edge-node-runtime-wire.md` — define identities, state, errors, limits, privacy, compatibility, and non-reuse rules.
|
||||
- [ ] `agent-spec/runtime/edge-node-execution.md` — synchronize implemented wire and explicit executor deferral.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Use a real registry and net-pipe client; reconnect the same Node id and assert the old binding never reaches the new client.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/edge/internal/service -run 'TestWorkspaceWire' -count=1`
|
||||
- `rg --sort path -n 'Workspace(Open|Tool|Cancel|Cleanup)|RunRequest|NodeCommand|generation|raw' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
- Expected: all sends are typed and generation-fenced; docs preserve the separate boundary.
|
||||
|
||||
## Modified Files Summary
|
||||
|
||||
| File | Item |
|
||||
|------|------|
|
||||
| `proto/iop/runtime.proto` | API-1 |
|
||||
| `proto/gen/iop/runtime.pb.go` | API-1 |
|
||||
| `apps/client/lib/gen/proto/iop/runtime.pb.dart` | API-1 |
|
||||
| `apps/client/lib/gen/proto/iop/runtime.pbenum.dart` | API-1 |
|
||||
| `apps/client/lib/gen/proto/iop/runtime.pbjson.dart` | API-1 |
|
||||
| `apps/client/lib/gen/proto/iop/runtime.pbserver.dart` | API-1 |
|
||||
| `apps/edge/internal/node/mapper.go` | API-1 |
|
||||
| `apps/edge/internal/node/mapper_test.go` | API-1 |
|
||||
| `apps/node/internal/transport/parser.go` | API-2 |
|
||||
| `apps/node/internal/transport/parser_test.go` | API-2 |
|
||||
| `apps/node/internal/transport/session.go` | API-2 |
|
||||
| `apps/node/internal/transport/session_test.go` | API-2 |
|
||||
| `apps/edge/internal/transport/server.go` | API-2 |
|
||||
| `apps/edge/internal/transport/server_test.go` | API-2 |
|
||||
| `apps/edge/internal/service/workspace_wire.go` | API-3 |
|
||||
| `apps/edge/internal/service/workspace_wire_test.go` | API-3 |
|
||||
| `agent-contract/inner/edge-node-runtime-wire.md` | API-3 |
|
||||
| `agent-spec/runtime/edge-node-execution.md` | API-3 |
|
||||
| `agent-task/m-iop-owned-single-request-agent-execution/09+08_workspace_wire/CODE_REVIEW-cloud-G09.md` | API-1, API-2, API-3 |
|
||||
|
||||
## Final Verification
|
||||
|
||||
1. `test -f agent-task/m-iop-owned-single-request-agent-execution/08+03,07_workspace_admission/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/08+03,07_workspace_admission/complete.log' | wc -l)" -eq 1`
|
||||
2. `make proto`
|
||||
3. `make proto-dart`
|
||||
4. `git diff --exit-code -- proto/gen/iop/agent.pb.go proto/gen/iop/control.pb.go proto/gen/iop/job.pb.go proto/gen/iop/node.pb.go apps/client/lib/gen/proto/iop/{control,job,node}.{pb,pbenum,pbjson,pbserver}.dart`
|
||||
5. `go test -race ./apps/edge/internal/node ./apps/edge/internal/service ./apps/edge/internal/transport ./apps/node/internal/transport -run 'Test(BuildConfigPayload.*Workspace|WorkspaceWire|NodeParserMapWorkspace|SessionWorkspace|EdgeParserMapWorkspace)' -count=1`
|
||||
6. `go test ./apps/edge/internal/node ./apps/edge/internal/service ./apps/edge/internal/transport ./apps/node/internal/transport -count=1`
|
||||
7. `go vet ./apps/edge/internal/node ./apps/edge/internal/service ./apps/edge/internal/transport ./apps/node/internal/transport`
|
||||
8. `make client-test`
|
||||
9. `make client-build-web`
|
||||
10. `rg --sort path -n 'Workspace(Open|Tool|Cancel|Cleanup)|request_id|RunRequest|NodeCommand|generation|raw' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
11. `git diff --check`
|
||||
|
||||
Expected: the predecessor is uniquely complete; Go and Dart generation is reproducible and limited to the intended runtime bindings; client consumers still compile; all typed wire paths pass under race; immutable request identity and provider-contract separation remain explicit. Cached tests are not acceptable.
|
||||
|
||||
**After completing all code changes, fill implementation-owned sections in `CODE_REVIEW-*-G??.md`.**
|
||||
|
|
@ -0,0 +1,165 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/09+08_workspace_wire plan=0 tag=API milestone-task=tool-executor -->
|
||||
|
||||
# Code Review Reference - API
|
||||
|
||||
> **[IMPLEMENTING AGENT — READ FIRST] Filling in this file is the mandatory final step of implementation.**
|
||||
> The task is NOT complete until every implementation-owned section below is filled in.
|
||||
> Complete the `Implementation Checklist`; the final checklist item is mandatory before saving.
|
||||
> Fill implementation-owned sections, then stop with active files in place and report ready for review.
|
||||
> Execute the plan's selected root cause, scope, files, and dependency decisions as written. Do not choose another owner, narrow/expand the write boundary, or replace a fix with another verification attempt.
|
||||
> If implementation is blocked, record the exact blocker, attempted commands/output, and resume condition only in implementation-owned evidence fields.
|
||||
> Do not ask the user directly, present choices, call user-input tools, create control-plane stop files, or classify the next state.
|
||||
> Finalization (`Code Review Result`, log rename, `complete.log`, archive moves, `Review-Only Checklist`) is review-agent-only, even after compaction/resume.
|
||||
> Follow the ownership table at the bottom of this file for which sections you own.
|
||||
|
||||
## Overview
|
||||
|
||||
date=2026-08-06
|
||||
task=m-iop-owned-single-request-agent-execution/09+08_workspace_wire, plan=0, tag=API
|
||||
|
||||
## For the Review Agent
|
||||
|
||||
> **[REVIEW AGENT ONLY]** The finalization steps below are review-agent only. Implementing agents must not execute this section.
|
||||
|
||||
Compare implementation of each item against source files and verify that output in `Verification Results` matches code.
|
||||
Review completion means the following steps are finished:
|
||||
|
||||
1. Append verdict and `review_rework_count` / `evidence_integrity_failure` routing signals.
|
||||
2. Archive `CODE_REVIEW-cloud-G09.md` → `code_review_cloud_G09_0.log` and `PLAN-cloud-G08.md` → `plan_cloud_G08_0.log`.
|
||||
3. If PASS, write `complete.log` and move active task directory to `agent-task/archive/YYYY/MM/m-iop-owned-single-request-agent-execution/09+08_workspace_wire/`. If WARN/FAIL, fully write the next filesystem state required by the code-review skill.
|
||||
4. If PASS, preserve the first-line `milestone-task=tool-executor` metadata in `complete.log` and report it for runtime aggregation. Roadmap state evaluation belongs to `sync-milestone-workstate`.
|
||||
5. Check applicable `Review-Only Checklist` items at the final `.log` location before reporting.
|
||||
|
||||
---
|
||||
|
||||
## Implementation Item Completion
|
||||
|
||||
| Item | Status |
|
||||
|------|---------|
|
||||
| API-1 Define the workspace protocol and catalog payload | [ ] |
|
||||
| API-2 Register compatible parsers and optional Node handlers | [ ] |
|
||||
| API-3 Dispatch only to the admitted generation | [ ] |
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Define and generate a dedicated typed workspace config/open/tool/cancel/cleanup protocol with closed operations, identities, statuses, error codes, and bounded result fields.
|
||||
- [ ] Deliver approved capabilities in `NodeConfigPayload` and register backward-compatible Edge/Node parsers plus an optional Node workspace handler.
|
||||
- [ ] Implement a generation-fenced service wire client that never reselects a Node and propagates timeout/context cancellation without raw logging.
|
||||
- [ ] Prove proto generation cleanliness, parser/round-trip/cancel/stale-generation behavior, and synchronize the wire contract/living spec.
|
||||
- [ ] Run dependency, proto, focused race, package, vet, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Review-Only Checklist
|
||||
|
||||
> **[REVIEW AGENT ONLY]** Implementing agents must not modify or check this section.
|
||||
|
||||
- [ ] Append PASS/WARN/FAIL with verified routing signals and matching findings/dimensions.
|
||||
- [ ] Archive the active pair to the routed `*_0.log` names.
|
||||
- [ ] Verify the managed `.gitignore` block.
|
||||
- [ ] On PASS write `complete.log`, preserve/report Milestone metadata, and move this directory to the monthly archive.
|
||||
- [ ] Keep the active task-group parent while siblings remain.
|
||||
- [ ] On WARN/FAIL write only the code-review skill's required next state.
|
||||
|
||||
## Deviations from Plan
|
||||
|
||||
_Record deviations and rationale here._
|
||||
|
||||
## Key Design Decisions
|
||||
|
||||
_Record key implemented decisions here._
|
||||
|
||||
## Reviewer Checkpoints
|
||||
|
||||
- Confirm no workspace data was added to `RunRequest`, provider execution, or `NodeCommand`.
|
||||
- Confirm protobuf field numbering, oneof use, closed enums, bounds, and generated-file provenance.
|
||||
- Confirm existing `Handler` mocks remain source-compatible through an optional interface.
|
||||
- Confirm every dispatch checks admitted Node id/generation and never reselects after reconnect.
|
||||
- Confirm logs/errors do not expose raw tool inputs/results or config secrets.
|
||||
|
||||
## Verification Results
|
||||
|
||||
Paste actual stdout/stderr for every command; record any replacement under deviations.
|
||||
|
||||
### 1. Dependency
|
||||
|
||||
`test -f agent-task/m-iop-owned-single-request-agent-execution/08+03,07_workspace_admission/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/08+03,07_workspace_admission/complete.log' | wc -l)" -eq 1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 2. Protobuf generation
|
||||
|
||||
`make proto`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 3. Generated-file scope
|
||||
|
||||
`git diff --exit-code -- proto/gen/iop/agent.pb.go proto/gen/iop/control.pb.go proto/gen/iop/job.pb.go proto/gen/iop/node.pb.go`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 4. Focused race tests
|
||||
|
||||
`go test -race ./apps/edge/internal/node ./apps/edge/internal/service ./apps/edge/internal/transport ./apps/node/internal/transport -run 'Test(BuildConfigPayload.*Workspace|WorkspaceWire|NodeParserMapWorkspace|SessionWorkspace|EdgeParserMapWorkspace)' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 5. Package regression
|
||||
|
||||
`go test ./apps/edge/internal/node ./apps/edge/internal/service ./apps/edge/internal/transport ./apps/node/internal/transport -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 6. Vet
|
||||
|
||||
`go vet ./apps/edge/internal/node ./apps/edge/internal/service ./apps/edge/internal/transport ./apps/node/internal/transport`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 7. Contract/spec search
|
||||
|
||||
`rg --sort path -n 'Workspace(Open|Tool|Cancel|Cleanup)|RunRequest|NodeCommand|generation|raw' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 8. Whitespace
|
||||
|
||||
`git diff --check`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
> **[IMPLEMENTING AGENT — BEFORE SAVING] Have you filled in every implementation-owned section?**
|
||||
> If anything is blank, go back and fill it in before saving this file.
|
||||
> Leave review-agent-only sections unchanged.
|
||||
|
||||
## Section Ownership
|
||||
|
||||
| Section | Owner | Note |
|
||||
|---------|-------|------|
|
||||
| Header comment, Overview, Review Agent Instructions | Fixed at stub creation | Implementing agent must not modify or execute these (archive, complete.log, and task-directory archive move are review-agent only) |
|
||||
| Archive Evidence Snapshot | Fixed at stub creation from plan when present | Implementing agent uses it as default prior-loop context; read only the specific archive files cited there when more detail is required |
|
||||
| Implementation Item Completion (item names) | Fixed at stub creation | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Implementation Checklist (item text/order) | Fixed at stub creation from plan | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Review-Only Checklist | Review agent only | Implementing agent must not modify or check this section |
|
||||
| Deviations from Plan, Key Design Decisions | Implementing agent | Replace placeholder text with actual content |
|
||||
| Reviewer Checkpoints | Fixed at stub creation | Pre-filled from plan |
|
||||
| Verification Results (section headings + commands) | Fixed at stub creation | Implementing agent fills in command output only; command changes require a `Deviations from Plan` entry |
|
||||
| Code Review Result | Review agent appends | Not included in stub |
|
||||
|
|
@ -0,0 +1,271 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/09+08_workspace_wire plan=0 tag=API milestone-task=tool-executor -->
|
||||
|
||||
# Dedicated Edge-Node Workspace Runtime Wire
|
||||
|
||||
## For the Implementing Agent
|
||||
|
||||
Do not start until packet 08 has `complete.log`. Keep all changes inside the listed boundary, generate protobuf only with `make proto`, run every command, fill the paired review stub, and leave review/finalization artifacts to the official reviewer.
|
||||
|
||||
## Background
|
||||
|
||||
The admitted binding needs a transport that is distinct from provider `RunRequest`, provider execution, and closed `NodeCommand`. This packet defines and proves that typed boundary, including catalog delivery, open/tool/cancel/cleanup requests, generation fencing, and bounded replies; Node execution remains unsupported until packet 10.
|
||||
|
||||
## Analysis
|
||||
|
||||
### Files Read
|
||||
|
||||
- `AGENTS.md`
|
||||
- `agent-ops/rules/project/rules.md`
|
||||
- `agent-ops/rules/common/rules-roadmap.md`
|
||||
- `agent-ops/rules/common/rules-agent-spec.md`
|
||||
- `agent-ops/rules/project/domain/edge/rules.md`
|
||||
- `agent-ops/rules/project/domain/node/rules.md`
|
||||
- `agent-ops/rules/project/domain/platform-common/rules.md`
|
||||
- `agent-test/local/rules.md`
|
||||
- `agent-test/local/edge-smoke.md`
|
||||
- `agent-test/local/node-smoke.md`
|
||||
- `agent-roadmap/phase/knowledge-tool-optimization-extension/milestones/iop-owned-single-request-agent-execution.md`
|
||||
- `agent-roadmap/sdd/knowledge-tool-optimization-extension/iop-owned-single-request-agent-execution/SDD.md`
|
||||
- `proto/iop/runtime.proto`
|
||||
- `apps/edge/internal/node/mapper.go`
|
||||
- `apps/edge/internal/node/mapper_test.go`
|
||||
- `apps/edge/internal/service/service.go`
|
||||
- `apps/edge/internal/service/node_command.go`
|
||||
- `apps/edge/internal/node/registry.go`
|
||||
- `apps/edge/internal/transport/server.go`
|
||||
- `apps/edge/internal/transport/server_test.go`
|
||||
- `apps/node/internal/transport/parser.go`
|
||||
- `apps/node/internal/transport/parser_test.go`
|
||||
- `apps/node/internal/transport/session.go`
|
||||
- `apps/node/internal/transport/session_test.go`
|
||||
- `agent-contract/inner/edge-node-runtime-wire.md`
|
||||
- `agent-contract/inner/execution-runtime.md`
|
||||
- `agent-spec/runtime/edge-node-execution.md`
|
||||
|
||||
### SDD Criteria
|
||||
|
||||
- D08 and S05 require a separate typed workspace request/result boundary, never `RunRequest.metadata`, provider execution, or `NodeCommand` extension.
|
||||
- Requests/results must cover success/error/timeout/large output and process cancellation with immutable request/stage/tool identity.
|
||||
- S07 requires cleanup to be an explicit request-owned action; execution semantics follow in packet 13.
|
||||
|
||||
### Verification Context
|
||||
|
||||
- `make -n proto` resolves to `protoc --go_out=... proto/iop/runtime.proto ...`; `protoc` is installed.
|
||||
- Baseline Edge/Node transport and service packages passed fresh.
|
||||
- Packet 08 supplies the exact admitted Node id/generation DTO. Wire tests can use `net.Pipe` and do not require a Mac filesystem.
|
||||
|
||||
### State and Concurrency Findings
|
||||
|
||||
- Each send must use the admitted Node id and generation; reconnect must fail as stale, not re-resolve.
|
||||
- Context cancellation sends typed cancel and leaves any request waiter bounded by its timeout.
|
||||
- Transport error messages/log fields must not include path, content, argv/template, environment values, stdout/stderr, or credentials.
|
||||
|
||||
### Test Coverage Gaps
|
||||
|
||||
- Parser maps and Session have no workspace message types or optional workspace handler.
|
||||
- `NodeConfigPayload` cannot carry the approved catalog and Edge has no generation-fenced typed client.
|
||||
|
||||
### Symbol References
|
||||
|
||||
- Keep `transport.Handler` source-compatible for all existing mocks by adding a separate optional `WorkspaceHandler` interface and type assertion.
|
||||
- `RunRequest` reserved workspace fields and `NodeCommand` enum remain untouched.
|
||||
- Generated output for `runtime.proto` is exactly `proto/gen/iop/runtime.pb.go`.
|
||||
|
||||
### Split Judgment
|
||||
|
||||
- Stable contract: proto generation, parser registration, optional handler behavior, and a net-pipe round trip independently PASS before filesystem effects.
|
||||
- Executor behavior remains packet 10 to keep wire verification deterministic and host-neutral.
|
||||
|
||||
### Scope Rationale
|
||||
|
||||
- Include proto/config payload, Edge client, parser/listener registration, tests, contract, and spec.
|
||||
- Exclude filesystem/process implementation, stage-provider decoding, cleanup effects, and public API output.
|
||||
|
||||
### Final Routing
|
||||
|
||||
- `evaluation_mode=first-pass`; build closures true, scores 2/1/2/1/2 = G08.
|
||||
- Finalizer route `risk-boundary`, lane `cloud`, filename `PLAN-cloud-G08.md`; positive risks are all five routing signatures.
|
||||
- Review scores 2/1/2/2/2 = G09; official review filename `CODE_REVIEW-cloud-G09.md`. No capability gap, rework, or evidence-integrity failure.
|
||||
|
||||
## Dependencies and Execution Order
|
||||
|
||||
1. Require packet 08 completion.
|
||||
2. Add source proto messages and generate Go.
|
||||
3. Map workspace catalog into registration payload, then register parsers/listeners.
|
||||
4. Add the generation-fenced Edge client and round-trip tests before docs.
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Define and generate a dedicated typed workspace config/open/tool/cancel/cleanup protocol with closed operations, identities, statuses, error codes, and bounded result fields.
|
||||
- [ ] Deliver approved capabilities in `NodeConfigPayload` and register backward-compatible Edge/Node parsers plus an optional Node workspace handler.
|
||||
- [ ] Implement a generation-fenced service wire client that never reselects a Node and propagates timeout/context cancellation without raw logging.
|
||||
- [ ] Prove proto generation cleanliness, parser/round-trip/cancel/stale-generation behavior, and synchronize the wire contract/living spec.
|
||||
- [ ] Run dependency, proto, focused race, package, vet, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Implementation Plan
|
||||
|
||||
### [API-1] Define the workspace protocol and catalog payload
|
||||
|
||||
**Problem**
|
||||
|
||||
- `proto/iop/runtime.proto:298` carries only adapters/runtime.
|
||||
- `proto/iop/runtime.proto:10` explicitly reserves legacy workspace fields on `RunRequest`, and `NodeCommandRequest` is a closed ops surface.
|
||||
|
||||
**Solution**
|
||||
|
||||
Before (`proto/iop/runtime.proto:298`):
|
||||
|
||||
```proto
|
||||
message NodeConfigPayload {
|
||||
repeated AdapterConfig adapters = 1;
|
||||
NodeRuntimeConfig runtime = 2;
|
||||
}
|
||||
```
|
||||
|
||||
After, add a new field and separate top-level protocol messages:
|
||||
|
||||
```proto
|
||||
message NodeConfigPayload {
|
||||
repeated AdapterConfig adapters = 1;
|
||||
NodeRuntimeConfig runtime = 2;
|
||||
repeated WorkspaceConfig workspaces = 3;
|
||||
}
|
||||
|
||||
message WorkspaceOpenRequest { /* execution_id, workspace_ref, limits */ }
|
||||
message WorkspaceToolRequest { /* execution/stage/tool ids, closed operation, typed input */ }
|
||||
message WorkspaceCancelRequest { /* execution_id, tool_call_id */ }
|
||||
message WorkspaceCleanupRequest { /* execution_id */ }
|
||||
```
|
||||
|
||||
`WorkspaceConfig` carries platform/root, closed operations, fixed command definitions, env names, and hard caps from packet 07. Tool input uses a proto `oneof` for relative path, write content, or command id/environment map. Responses echo identities and use closed status/error-code enums with typed bounded content/list/stdout/stderr, exit code, truncation, duration, and cleanup counts. Reserve no caller-selected Node/root/executable/argv field. Preserve existing field numbers and never reuse reservations.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `proto/iop/runtime.proto` — define catalog and four request/response families.
|
||||
- [ ] `proto/gen/iop/runtime.pb.go` — regenerate with `make proto`; no hand edits.
|
||||
- [ ] `apps/edge/internal/node/mapper.go` — serialize packet 07 workspace definitions into the private Node payload.
|
||||
- [ ] `apps/edge/internal/node/mapper_test.go` — assert complete typed mapping and no legacy settings leakage.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Extend mapper tests and rely on parser round trips in API-2 for every new proto family.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `make proto`
|
||||
- `git diff --exit-code -- proto/gen/iop/agent.pb.go proto/gen/iop/control.pb.go proto/gen/iop/job.pb.go proto/gen/iop/node.pb.go`
|
||||
- `go test ./apps/edge/internal/node -run 'TestBuildConfigPayload.*Workspace' -count=1`
|
||||
- Expected: only `runtime.pb.go` changes and the payload retains the complete approved catalog.
|
||||
|
||||
### [API-2] Register compatible parsers and optional Node handlers
|
||||
|
||||
**Problem**
|
||||
|
||||
- `apps/node/internal/transport/session.go:17` requires every handler to implement the provider methods, so adding workspace methods there would break all mocks.
|
||||
- `apps/edge/internal/transport/server.go:35` and the Node parser map do not decode new request/response types.
|
||||
|
||||
**Solution**
|
||||
|
||||
Before (`apps/node/internal/transport/session.go:17`):
|
||||
|
||||
```go
|
||||
type Handler interface {
|
||||
OnRunRequest(context.Context, *Session, *iop.RunRequest) error
|
||||
// Existing provider methods.
|
||||
}
|
||||
```
|
||||
|
||||
After, leave `Handler` unchanged and add:
|
||||
|
||||
```go
|
||||
type WorkspaceHandler interface {
|
||||
OnWorkspaceOpen(context.Context, *Session, *iop.WorkspaceOpenRequest) (*iop.WorkspaceOpenResponse, error)
|
||||
OnWorkspaceTool(context.Context, *Session, *iop.WorkspaceToolRequest) (*iop.WorkspaceToolResponse, error)
|
||||
OnWorkspaceCancel(context.Context, *Session, *iop.WorkspaceCancelRequest) (*iop.WorkspaceCancelResponse, error)
|
||||
OnWorkspaceCleanup(context.Context, *Session, *iop.WorkspaceCleanupRequest) (*iop.WorkspaceCleanupResponse, error)
|
||||
}
|
||||
```
|
||||
|
||||
Register request listeners that type-assert `WorkspaceHandler` and return a typed unsupported/not-ready response when absent or on handler error. Register all request parsers on Node and response parsers on Edge. Do not log raw request/response fields.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/node/internal/transport/parser.go` — parse workspace requests.
|
||||
- [ ] `apps/node/internal/transport/parser_test.go` — round-trip every request shape.
|
||||
- [ ] `apps/node/internal/transport/session.go` — add optional handler/listeners and typed failure translation.
|
||||
- [ ] `apps/node/internal/transport/session_test.go` — net-pipe success, absent handler, error, and identity echo cases.
|
||||
- [ ] `apps/edge/internal/transport/server.go` — parse workspace responses.
|
||||
- [ ] `apps/edge/internal/transport/server_test.go` — round-trip every response shape.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Named tests `TestNodeParserMapWorkspace`, `TestSessionWorkspaceRequest`, and `TestEdgeParserMapWorkspace` cover all message families and preserve existing handler compile compatibility.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/node/internal/transport ./apps/edge/internal/transport -run 'Test(NodeParserMapWorkspace|SessionWorkspace|EdgeParserMapWorkspace)' -count=1`
|
||||
- Expected: typed round trips succeed; absent handlers return typed failure without panic or raw leakage.
|
||||
|
||||
### [API-3] Dispatch only to the admitted generation
|
||||
|
||||
**Problem**
|
||||
|
||||
- `apps/edge/internal/service/node_command.go:122` resolves a Node per call and has no request-stable workspace generation.
|
||||
- Context cancellation has no workspace-specific cancel/cleanup path.
|
||||
|
||||
**Solution**
|
||||
|
||||
Add `workspace_wire.go` in service with `workspaceOpen`, `workspaceTool`, `workspaceCancel`, and `workspaceCleanup` methods. Each method accepts packet 08's frozen binding, obtains `ReadyOwnerSnapshot(binding.NodeID)`, compares `ConnectionGeneration`, and sends to that exact client only. Tool/open wait uses the lower admitted deadline; a cancelled context sends typed cancel once and all goroutines remain bounded by transport timeout. Translate transport/stale/typed Node errors to stable internal errors without including raw payload.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/edge/internal/service/workspace_wire.go` — implement exact-generation typed request/response dispatch.
|
||||
- [ ] `apps/edge/internal/service/workspace_wire_test.go` — net-pipe open/tool/cancel/cleanup, timeout, cancellation, stale generation, and no-reselection tests.
|
||||
- [ ] `agent-contract/inner/edge-node-runtime-wire.md` — define identities, state, errors, limits, privacy, compatibility, and non-reuse rules.
|
||||
- [ ] `agent-spec/runtime/edge-node-execution.md` — synchronize implemented wire and explicit executor deferral.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Use a real registry and net-pipe client; reconnect the same Node id and assert the old binding never reaches the new client.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/edge/internal/service -run 'TestWorkspaceWire' -count=1`
|
||||
- `rg --sort path -n 'Workspace(Open|Tool|Cancel|Cleanup)|RunRequest|NodeCommand|generation|raw' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
- Expected: all sends are typed and generation-fenced; docs preserve the separate boundary.
|
||||
|
||||
## Modified Files Summary
|
||||
|
||||
| File | Item |
|
||||
|------|------|
|
||||
| `proto/iop/runtime.proto` | API-1 |
|
||||
| `proto/gen/iop/runtime.pb.go` | API-1 |
|
||||
| `apps/edge/internal/node/mapper.go` | API-1 |
|
||||
| `apps/edge/internal/node/mapper_test.go` | API-1 |
|
||||
| `apps/node/internal/transport/parser.go` | API-2 |
|
||||
| `apps/node/internal/transport/parser_test.go` | API-2 |
|
||||
| `apps/node/internal/transport/session.go` | API-2 |
|
||||
| `apps/node/internal/transport/session_test.go` | API-2 |
|
||||
| `apps/edge/internal/transport/server.go` | API-2 |
|
||||
| `apps/edge/internal/transport/server_test.go` | API-2 |
|
||||
| `apps/edge/internal/service/workspace_wire.go` | API-3 |
|
||||
| `apps/edge/internal/service/workspace_wire_test.go` | API-3 |
|
||||
| `agent-contract/inner/edge-node-runtime-wire.md` | API-3 |
|
||||
| `agent-spec/runtime/edge-node-execution.md` | API-3 |
|
||||
| `agent-task/m-iop-owned-single-request-agent-execution/09+08_workspace_wire/CODE_REVIEW-cloud-G09.md` | API-1, API-2, API-3 |
|
||||
|
||||
## Final Verification
|
||||
|
||||
1. `test -f agent-task/m-iop-owned-single-request-agent-execution/08+03,07_workspace_admission/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/08+03,07_workspace_admission/complete.log' | wc -l)" -eq 1`
|
||||
2. `make proto`
|
||||
3. `git diff --exit-code -- proto/gen/iop/agent.pb.go proto/gen/iop/control.pb.go proto/gen/iop/job.pb.go proto/gen/iop/node.pb.go`
|
||||
4. `go test -race ./apps/edge/internal/node ./apps/edge/internal/service ./apps/edge/internal/transport ./apps/node/internal/transport -run 'Test(BuildConfigPayload.*Workspace|WorkspaceWire|NodeParserMapWorkspace|SessionWorkspace|EdgeParserMapWorkspace)' -count=1`
|
||||
5. `go test ./apps/edge/internal/node ./apps/edge/internal/service ./apps/edge/internal/transport ./apps/node/internal/transport -count=1`
|
||||
6. `go vet ./apps/edge/internal/node ./apps/edge/internal/service ./apps/edge/internal/transport ./apps/node/internal/transport`
|
||||
7. `rg --sort path -n 'Workspace(Open|Tool|Cancel|Cleanup)|RunRequest|NodeCommand|generation|raw' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
8. `git diff --check`
|
||||
|
||||
Expected: the predecessor is uniquely complete; generation is reproducible and limited to the intended generated file; all typed wire paths pass under race; provider contracts remain separate. Cached tests are not acceptable.
|
||||
|
||||
**After completing all code changes, fill implementation-owned sections in `CODE_REVIEW-*-G??.md`.**
|
||||
|
|
@ -0,0 +1,168 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/10+09_workspace_files plan=1 tag=API milestone-task=tool-executor -->
|
||||
|
||||
# Code Review Reference - API
|
||||
|
||||
> **[IMPLEMENTING AGENT — READ FIRST] Filling in this file is the mandatory final step of implementation.**
|
||||
> The task is NOT complete until every implementation-owned section below is filled in.
|
||||
> Complete the `Implementation Checklist`; the final checklist item is mandatory before saving.
|
||||
> Fill implementation-owned sections, then stop with active files in place and report ready for review.
|
||||
> Execute the plan's selected root cause, scope, files, and dependency decisions as written. Do not choose another owner, narrow/expand the write boundary, or replace a fix with another verification attempt.
|
||||
> If implementation is blocked, record the exact blocker, attempted commands/output, and resume condition only in implementation-owned evidence fields.
|
||||
> Do not ask the user directly, present choices, call user-input tools, create control-plane stop files, or classify the next state.
|
||||
> Finalization (`Code Review Result`, log rename, `complete.log`, archive moves, `Review-Only Checklist`) is review-agent-only, even after compaction/resume.
|
||||
> Follow the ownership table at the bottom of this file for which sections you own.
|
||||
|
||||
## Overview
|
||||
|
||||
date=2026-08-06
|
||||
task=m-iop-owned-single-request-agent-execution/10+09_workspace_files, plan=1, tag=API
|
||||
|
||||
## Archive Evidence Snapshot
|
||||
|
||||
- The first-pass pair is preserved at `agent-task/m-iop-owned-single-request-agent-execution/10+09_workspace_files/plan_cloud_G08_0.log` and `agent-task/m-iop-owned-single-request-agent-execution/10+09_workspace_files/code_review_cloud_G09_0.log`; it contains no implementation evidence or review verdict.
|
||||
- Self-review found that the original reserved-root wording did not isolate sibling requests and used a second execution identity. Plan 1 binds the immutable coordinator `request_id`, reserves only `.iop/job/<request_id>` for internal runtime use, denies all caller access to `.iop`, and adds an independent Darwin compile gate.
|
||||
|
||||
## For the Review Agent
|
||||
|
||||
> **[REVIEW AGENT ONLY]** The finalization steps below are review-agent only. Implementing agents must not execute this section.
|
||||
|
||||
Compare implementation of each item against source files and verify that output in `Verification Results` matches code.
|
||||
Review completion means the following steps are finished:
|
||||
|
||||
1. Append verdict and `review_rework_count` / `evidence_integrity_failure` routing signals.
|
||||
2. Archive `CODE_REVIEW-cloud-G09.md` → `code_review_cloud_G09_1.log` and `PLAN-cloud-G08.md` → `plan_cloud_G08_1.log`.
|
||||
3. If PASS, write `complete.log` and move active task directory to `agent-task/archive/YYYY/MM/m-iop-owned-single-request-agent-execution/10+09_workspace_files/`. If WARN/FAIL, fully write the next filesystem state required by the code-review skill.
|
||||
4. If PASS, preserve the first-line `milestone-task=tool-executor` metadata in `complete.log` and report it for runtime aggregation. Roadmap state evaluation belongs to `sync-milestone-workstate`.
|
||||
5. Check applicable `Review-Only Checklist` items at the final `.log` location before reporting.
|
||||
|
||||
---
|
||||
|
||||
## Implementation Item Completion
|
||||
|
||||
| Item | Status |
|
||||
|------|---------|
|
||||
| API-1 Own immutable roots and request contexts | [ ] |
|
||||
| API-2 Execute canonical bounded file operations | [ ] |
|
||||
| API-3 Wire Node handler and bootstrap lifecycle | [ ] |
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Build a Mac-only immutable workspace catalog using `os.Root`, canonical-root checks, immutable coordinator `request_id` binding, and explicit runtime lifecycle ownership.
|
||||
- [ ] Implement bounded read/list plus atomic write and non-recursive delete with fail-closed relative path, symlink, mount, special-file, capability, and `.iop` namespace validation.
|
||||
- [ ] Implement packet 09's optional Node workspace handler, bootstrap/close the runtime before ready, and keep command typed-unsupported.
|
||||
- [ ] Prove containment, sibling-request isolation, bounds, concurrency, mapping, startup failure, and synchronize only implemented file-executor contract/spec claims.
|
||||
- [ ] Run dependency, focused race, package, vet, Darwin compile, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Review-Only Checklist
|
||||
|
||||
> **[REVIEW AGENT ONLY]** Implementing agents must not modify/check this section.
|
||||
|
||||
- [ ] Append PASS/WARN/FAIL, routing signals, dimensions, and findings.
|
||||
- [ ] Archive the routed active pair to suffix `1` logs.
|
||||
- [ ] Verify managed `.gitignore` entries.
|
||||
- [ ] On PASS write `complete.log`, preserve/report Milestone metadata, move this directory, and keep the active parent while siblings remain.
|
||||
- [ ] On WARN/FAIL create only the required next loop state.
|
||||
|
||||
## Deviations from Plan
|
||||
|
||||
_Record deviations and rationale._
|
||||
|
||||
## Key Design Decisions
|
||||
|
||||
_Record implemented decisions._
|
||||
|
||||
## Reviewer Checkpoints
|
||||
|
||||
- Confirm opened `os.Root`/directory handles are the only filesystem authority, root itself is canonical/non-symlink, opened targets do not cross the admitted filesystem identity, and later command cwd cannot re-resolve a replaced configured path.
|
||||
- Confirm read/list allocation is bounded and write is same-directory atomic with no partial target.
|
||||
- Confirm caller access to `.iop`, sibling job namespaces, mount traversal, escape symlinks, absolute/parent paths, special files, root delete, recursive delete, and unsupported commands fail closed.
|
||||
- Confirm bootstrap owns and closes roots before ready/reconnect teardown and errors/logs remain raw-free.
|
||||
|
||||
## Verification Results
|
||||
|
||||
Paste actual stdout/stderr for every command; record replacements under deviations.
|
||||
|
||||
### 1. Dependency
|
||||
|
||||
`test -f agent-task/m-iop-owned-single-request-agent-execution/09+08_workspace_wire/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/09+08_workspace_wire/complete.log' | wc -l)" -eq 1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 2. Runtime/file race tests
|
||||
|
||||
`go test -race ./apps/node/internal/workspace -run 'Test(Runtime|FileExecutor)' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 3. Node/bootstrap race tests
|
||||
|
||||
`go test -race ./apps/node/internal/node ./apps/node/internal/bootstrap -run 'Test(NodeWorkspace|WorkspaceRuntime)' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 4. Package regression
|
||||
|
||||
`go test ./apps/node/internal/workspace ./apps/node/internal/node ./apps/node/internal/bootstrap ./apps/node/internal/transport -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 5. Vet
|
||||
|
||||
`go vet ./apps/node/internal/workspace ./apps/node/internal/node ./apps/node/internal/bootstrap`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 6. Darwin compile
|
||||
|
||||
`GOOS=darwin GOARCH=arm64 go test -c -o /tmp/iop-workspace-files-darwin.test ./apps/node/internal/workspace`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 7. Contract/spec search
|
||||
|
||||
`rg --sort path -n 'os.Root|request_id|\.iop/job|read|list|write|delete|symlink|mount|command.*defer|cleanup.*defer' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 8. Whitespace
|
||||
|
||||
`git diff --check`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
> **[IMPLEMENTING AGENT — BEFORE SAVING] Have you filled in every implementation-owned section?**
|
||||
> If anything is blank, go back and fill it in before saving this file.
|
||||
> Leave review-agent-only sections unchanged.
|
||||
|
||||
## Section Ownership
|
||||
|
||||
| Section | Owner | Note |
|
||||
|---------|-------|------|
|
||||
| Header comment, Overview, Review Agent Instructions | Fixed at stub creation | Implementing agent must not modify or execute these (archive, complete.log, and task-directory archive move are review-agent only) |
|
||||
| Archive Evidence Snapshot | Fixed at stub creation from plan when present | Implementing agent uses it as default prior-loop context; read only the specific archive files cited there when more detail is required |
|
||||
| Implementation Item Completion (item names) | Fixed at stub creation | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Implementation Checklist (item text/order) | Fixed at stub creation from plan | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Review-Only Checklist | Review agent only | Implementing agent must not modify or check this section |
|
||||
| Deviations from Plan, Key Design Decisions | Implementing agent | Replace placeholder text with actual content |
|
||||
| Reviewer Checkpoints | Fixed at stub creation | Pre-filled from plan |
|
||||
| Verification Results (section headings + commands) | Fixed at stub creation | Implementing agent fills in command output only; command changes require a `Deviations from Plan` entry |
|
||||
| Code Review Result | Review agent appends | Not included in stub |
|
||||
|
|
@ -0,0 +1,257 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/10+09_workspace_files plan=1 tag=API milestone-task=tool-executor -->
|
||||
|
||||
# Mac Workspace File Executor
|
||||
|
||||
## For the Implementing Agent
|
||||
|
||||
Do not start until packet 09 has `complete.log`. Implement only the exact files listed, preserve the typed wire contract, run every verification command, and fill `CODE_REVIEW-cloud-G09.md`. Official review owns verdict, logs, completion, and archive moves.
|
||||
|
||||
## Background
|
||||
|
||||
The dedicated wire is intentionally inert until Node can validate its private catalog, open a request context, and execute canonical read/list/write/delete operations beneath one root. This packet supplies that Mac-owned filesystem boundary and leaves command/process execution to packet 11.
|
||||
|
||||
## Archive Evidence Snapshot
|
||||
|
||||
- The first-pass pair is preserved at `agent-task/m-iop-owned-single-request-agent-execution/10+09_workspace_files/plan_cloud_G08_0.log` and `agent-task/m-iop-owned-single-request-agent-execution/10+09_workspace_files/code_review_cloud_G09_0.log`; it contains no implementation evidence or review verdict.
|
||||
- Self-review found that the original reserved-root wording did not isolate sibling requests and used a second execution identity. Plan 1 binds the immutable coordinator `request_id`, reserves only `.iop/job/<request_id>` for internal runtime use, denies all caller access to `.iop`, and adds an independent Darwin compile gate.
|
||||
|
||||
## Analysis
|
||||
|
||||
### Files Read
|
||||
|
||||
- `AGENTS.md`
|
||||
- `agent-ops/rules/project/rules.md`
|
||||
- `agent-ops/rules/common/rules-roadmap.md`
|
||||
- `agent-ops/rules/common/rules-agent-spec.md`
|
||||
- `agent-ops/rules/project/domain/node/rules.md`
|
||||
- `agent-ops/rules/project/domain/platform-common/rules.md`
|
||||
- `agent-test/local/rules.md`
|
||||
- `agent-test/local/node-smoke.md`
|
||||
- `agent-roadmap/phase/knowledge-tool-optimization-extension/milestones/iop-owned-single-request-agent-execution.md`
|
||||
- `agent-roadmap/sdd/knowledge-tool-optimization-extension/iop-owned-single-request-agent-execution/SDD.md`
|
||||
- `apps/node/internal/bootstrap/module.go`
|
||||
- `apps/node/internal/bootstrap/module_test.go`
|
||||
- `apps/node/internal/node/node.go`
|
||||
- `apps/node/internal/node/config_refresh_handler.go`
|
||||
- `apps/node/internal/transport/session.go`
|
||||
- `apps/edge/internal/node/mapper.go`
|
||||
- `proto/iop/runtime.proto`
|
||||
- `agent-contract/inner/edge-node-runtime-wire.md`
|
||||
- `agent-contract/inner/execution-runtime.md`
|
||||
- `agent-spec/runtime/edge-node-execution.md`
|
||||
|
||||
### SDD Criteria
|
||||
|
||||
- S04 requires approved root containment and rejection of absolute/foreign paths, caller access to `.iop`, sibling request namespaces, mount-boundary traversal, and symlink escape before effects.
|
||||
- S05 requires typed read/list/write/delete success/failure and bounded output. D06 excludes interactive shell/desktop/scheduler.
|
||||
- Cleanup and command process groups remain packets 13 and 11 respectively.
|
||||
|
||||
### Verification Context
|
||||
|
||||
- Host Go is 1.26.2; module baseline supports Go 1.24, whose `os.Root` provides root-relative APIs and rejects symlink traversal outside the root.
|
||||
- `go doc os.Root` confirms methods are concurrent-safe and that special files/mounts still require explicit rejection.
|
||||
- Baseline Node/bootstrap/transport packages passed fresh. Filesystem tests use `t.TempDir`; production Mac platform validation is injected with an explicit host-OS argument for deterministic Linux CI tests.
|
||||
|
||||
### State and Concurrency Findings
|
||||
|
||||
- One runtime catalog owns long-lived `os.Root` handles plus opened directory handles for stable process cwd; request contexts bind the immutable coordinator `request_id` to exactly one ref and immutable caps. No Node-local execution-id alias is introduced.
|
||||
- Duplicate open is idempotent only for byte-identical binding; conflicting reuse fails. A request cannot operate before open or after close/cleanup.
|
||||
- `os.Root` supplies lexical/symlink containment but explicitly does not prohibit mount traversal. The executor must separately reject empty/absolute/parent paths, root deletion, caller-visible `.iop`, sibling jobs, cross-device/mount targets, non-regular reads/writes, devices/FIFOs, recursive user delete, and unbounded reads/lists.
|
||||
|
||||
### Test Coverage Gaps
|
||||
|
||||
- Node has no workspace runtime field/handler or bootstrap catalog validation.
|
||||
- No test covers in-root symlinks versus escaping symlinks, special files, bounded listing/read, atomic writes, or concurrent request isolation.
|
||||
|
||||
### Symbol References
|
||||
|
||||
- Packet 09's optional `transport.WorkspaceHandler` is implemented by `*node.Node`; existing `transport.Handler` remains unchanged.
|
||||
- `runtimeOwner.close` must close workspace roots during reconnect/shutdown.
|
||||
- No provider adapter/router/store API is reused.
|
||||
|
||||
### Split Judgment
|
||||
|
||||
- Stable contract: file-only runtime independently passes every S04 containment case and returns typed unsupported for command.
|
||||
- Command is separate because process-group cancellation and environment/output races are a distinct correctness boundary.
|
||||
|
||||
### Scope Rationale
|
||||
|
||||
- Include catalog startup validation, request contexts, file operations, Node handler, bootstrap lifecycle, tests, contract/spec sync.
|
||||
- Exclude command execution, provider tool loop, cleanup artifact removal, and metrics.
|
||||
|
||||
### Final Routing
|
||||
|
||||
- `evaluation_mode=isolated-reassessment`; `review_rework_count=0`; `evidence_integrity_failure=false`; build closures true, scores 2/1/2/1/2 = G08.
|
||||
- Finalizer route `risk-boundary`, lane `cloud`, filename `PLAN-cloud-G08.md`; risks `boundary_contract`, `structured_interpretation`, `variant_product`, `concurrent_consistency` (4).
|
||||
- Review scores 2/1/2/2/2 = G09; official filename `CODE_REVIEW-cloud-G09.md`; no capability gap or recovery signal.
|
||||
|
||||
## Dependencies and Execution Order
|
||||
|
||||
1. Require packet 09 completion.
|
||||
2. Implement the private catalog/root layer and file operations.
|
||||
3. Adapt typed messages in Node, then wire runtime ownership in bootstrap.
|
||||
4. Run race/containment tests before updating contract/spec claims.
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Build a Mac-only immutable workspace catalog using `os.Root`, canonical-root checks, immutable coordinator `request_id` binding, and explicit runtime lifecycle ownership.
|
||||
- [ ] Implement bounded read/list plus atomic write and non-recursive delete with fail-closed relative path, symlink, mount, special-file, capability, and `.iop` namespace validation.
|
||||
- [ ] Implement packet 09's optional Node workspace handler, bootstrap/close the runtime before ready, and keep command typed-unsupported.
|
||||
- [ ] Prove containment, sibling-request isolation, bounds, concurrency, mapping, startup failure, and synchronize only implemented file-executor contract/spec claims.
|
||||
- [ ] Run dependency, focused race, package, vet, Darwin compile, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Implementation Plan
|
||||
|
||||
### [API-1] Own immutable roots and request contexts
|
||||
|
||||
**Problem**
|
||||
|
||||
- `apps/node/internal/node/node.go:18` has only provider runtime fields.
|
||||
- `apps/node/internal/bootstrap/module.go:100` builds adapters immediately from registration config and signals ready without validating workspaces.
|
||||
|
||||
**Solution**
|
||||
|
||||
Before (`apps/node/internal/node/node.go:18`):
|
||||
|
||||
```go
|
||||
type Node struct {
|
||||
nodeID string
|
||||
router runtime.Router
|
||||
store *store.Store
|
||||
// provider fields
|
||||
}
|
||||
```
|
||||
|
||||
After:
|
||||
|
||||
```go
|
||||
type Node struct {
|
||||
// Existing fields remain.
|
||||
workspaceRuntime *workspace.Runtime
|
||||
}
|
||||
|
||||
func (n *Node) SetWorkspaceRuntime(rt *workspace.Runtime) { n.workspaceRuntime = rt }
|
||||
```
|
||||
|
||||
Create `apps/node/internal/workspace` with an immutable catalog. Constructor accepts packet 09 configs plus explicit host OS, requires `darwin`, validates the configured root exists, is an absolute directory and not a symlink/root, records its filesystem identity, and opens both `os.Root` and a directory handle for later descriptor-based command cwd. The opened handles, not a re-resolved path, are the admitted workspace authority. `Open` binds the validated immutable coordinator `request_id` to one workspace ref and copied lower limits; conflicting duplicate ids fail. The request context derives one internal prefix, `.iop/job/<request_id>`, from that validated identity and exposes no caller-controlled internal path. `Close` makes and closes all handles exactly once and is idempotent.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/node/internal/workspace/runtime.go` — catalog, filesystem and stable directory handles/identity, request map, open/close, immutable request identity, internal-prefix derivation, and capability checks.
|
||||
- [ ] `apps/node/internal/workspace/runtime_test.go` — platform/root/startup, duplicate/open/close, request-id validation, copy and concurrent request isolation.
|
||||
- [ ] `apps/node/internal/node/node.go` — hold the optional runtime without changing constructor call sites.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Test valid injected `darwin`, wrong host OS, root symlink, missing/non-directory/root path, duplicate refs, invalid/conflicting request id, and concurrent isolation under `-race`.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/node/internal/workspace -run 'TestRuntime(Open|Catalog|Close|Concurrent)' -count=1`
|
||||
- Expected: only a validated Mac catalog opens and request identities never cross roots.
|
||||
|
||||
### [API-2] Execute canonical bounded file operations
|
||||
|
||||
**Problem**
|
||||
|
||||
- No Node code executes relative workspace tools; using `os.ReadFile`/ordinary joins would allow unbounded allocation or TOCTOU escape.
|
||||
|
||||
**Solution**
|
||||
|
||||
Add path validation before every operation and use only the request's `*os.Root`. Caller tool paths must be canonical relative user paths and must reject `.iop` itself and every descendant before lookup. A separate unexported internal-path helper may accept only the exact derived `.iop/job/<request_id>` prefix for the current request; it rejects `.iop`, `.iop/job`, sibling ids, and caller-supplied variants. Opened targets/parents must remain on the admitted root filesystem and must not be symlinks or special files where the operation requires regular files/directories; fail closed when identity cannot be proven. Read through `io.LimitReader(max+1)`, stat the opened handle as regular, and return typed truncation/error. List a directory with stable lexical ordering, entry/type encoding, and shared byte/entry bound. Write rejects oversized input and non-regular existing targets, creates validated parents through `Root.MkdirAll`, writes/fsyncs a random same-directory temp, and atomically renames. Delete rejects `.` and removes only a regular file, symlink itself, or empty directory; never recursively deletes user paths. Command returns the packet 09 unsupported error until packet 11.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/node/internal/workspace/path.go` — canonical relative user path, exact request-owned internal-prefix, sibling namespace, filesystem-identity, and reserved-root validation.
|
||||
- [ ] `apps/node/internal/workspace/file_executor.go` — bounded read/list, atomic write, non-recursive delete, typed results.
|
||||
- [ ] `apps/node/internal/workspace/file_executor_test.go` — operation/capability matrix, bounds, `.iop` and sibling-request denial, symlinks/mounts/special files, atomic replacement, and concurrent roots.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Use `t.TempDir`, inside/outside symlinks, a mount substitute/helper where the host permits it, FIFO where supported, large fixtures, and parallel operations. Verify caller operations cannot observe or mutate `.iop`, the current request's internal helper cannot enter a sibling job, no cross-filesystem/outside file changes occur, and no partial target remains after failed write. Skip only the privileged mount fixture when unavailable while retaining deterministic filesystem-identity unit coverage.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/node/internal/workspace -run 'TestFileExecutor' -count=1`
|
||||
- Expected: all canonical user operations work inside root and every reserved-namespace/sibling/mount/escape/special/bound violation fails before outside or partial effects.
|
||||
|
||||
### [API-3] Wire Node handler and bootstrap lifecycle
|
||||
|
||||
**Problem**
|
||||
|
||||
- `apps/node/internal/bootstrap/module.go:124` creates Node and signals ready without workspace initialization.
|
||||
- Packet 09's optional handler currently returns unsupported because `Node` does not implement it.
|
||||
|
||||
**Solution**
|
||||
|
||||
Before (`apps/node/internal/bootstrap/module.go:124`):
|
||||
|
||||
```go
|
||||
rtr := router.New(set.Registry, logger)
|
||||
n := node.New(result.NodeID, rtr, st, globalConcurrency, os.Stdout, logger, set)
|
||||
```
|
||||
|
||||
After:
|
||||
|
||||
```go
|
||||
workspaceRuntime, err := workspace.NewRuntime(result.Config.GetWorkspaces(), runtime.GOOS, logger)
|
||||
if err != nil { /* close owner and fail before ready */ }
|
||||
n := node.New(/* existing args */)
|
||||
n.SetWorkspaceRuntime(workspaceRuntime)
|
||||
owner.workspace = workspaceRuntime
|
||||
```
|
||||
|
||||
Implement open/tool dispatch in `workspace_handler.go`, echo the immutable `request_id` plus stage/tool identities, map only stable error codes/messages, and never log request path/content/result. Cancel/cleanup remain typed unsupported. Extend `runtimeOwner.close` to close the workspace runtime before session/store teardown.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/node/internal/node/workspace_handler.go` — implement open/file tool responses and typed unsupported command/cancel/cleanup.
|
||||
- [ ] `apps/node/internal/node/workspace_handler_test.go` — handler mapping, missing runtime, identity, raw-free error cases.
|
||||
- [ ] `apps/node/internal/bootstrap/module.go` — construct/own/close runtime before `SignalReady`.
|
||||
- [ ] `apps/node/internal/bootstrap/workspace_runtime_test.go` — startup success/failure and close ownership with injected payload/host OS helper.
|
||||
- [ ] `agent-contract/inner/edge-node-runtime-wire.md` — mark file semantics implemented and command/cleanup deferred.
|
||||
- [ ] `agent-spec/runtime/edge-node-execution.md` — synchronize current runtime entry points and tests.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Use direct handler tests and a bootstrap composition helper; no real Edge or Mac host is required.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/node/internal/node ./apps/node/internal/bootstrap -run 'Test(NodeWorkspace|WorkspaceRuntime)' -count=1`
|
||||
- `rg --sort path -n 'os.Root|read|list|write|delete|symlink|command.*defer|cleanup.*defer' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
- Expected: Node becomes ready only after valid root ownership and docs claim file operations only.
|
||||
|
||||
## Modified Files Summary
|
||||
|
||||
| File | Item |
|
||||
|------|------|
|
||||
| `apps/node/internal/workspace/runtime.go` | API-1 |
|
||||
| `apps/node/internal/workspace/runtime_test.go` | API-1 |
|
||||
| `apps/node/internal/node/node.go` | API-1 |
|
||||
| `apps/node/internal/workspace/path.go` | API-2 |
|
||||
| `apps/node/internal/workspace/file_executor.go` | API-2 |
|
||||
| `apps/node/internal/workspace/file_executor_test.go` | API-2 |
|
||||
| `apps/node/internal/node/workspace_handler.go` | API-3 |
|
||||
| `apps/node/internal/node/workspace_handler_test.go` | API-3 |
|
||||
| `apps/node/internal/bootstrap/module.go` | API-3 |
|
||||
| `apps/node/internal/bootstrap/workspace_runtime_test.go` | API-3 |
|
||||
| `agent-contract/inner/edge-node-runtime-wire.md` | API-3 |
|
||||
| `agent-spec/runtime/edge-node-execution.md` | API-3 |
|
||||
| `agent-task/m-iop-owned-single-request-agent-execution/10+09_workspace_files/CODE_REVIEW-cloud-G09.md` | API-1, API-2, API-3 |
|
||||
|
||||
## Final Verification
|
||||
|
||||
1. `test -f agent-task/m-iop-owned-single-request-agent-execution/09+08_workspace_wire/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/09+08_workspace_wire/complete.log' | wc -l)" -eq 1`
|
||||
2. `go test -race ./apps/node/internal/workspace -run 'Test(Runtime|FileExecutor)' -count=1`
|
||||
3. `go test -race ./apps/node/internal/node ./apps/node/internal/bootstrap -run 'Test(NodeWorkspace|WorkspaceRuntime)' -count=1`
|
||||
4. `go test ./apps/node/internal/workspace ./apps/node/internal/node ./apps/node/internal/bootstrap ./apps/node/internal/transport -count=1`
|
||||
5. `go vet ./apps/node/internal/workspace ./apps/node/internal/node ./apps/node/internal/bootstrap`
|
||||
6. `GOOS=darwin GOARCH=arm64 go test -c -o /tmp/iop-workspace-files-darwin.test ./apps/node/internal/workspace`
|
||||
7. `rg --sort path -n 'os.Root|request_id|\.iop/job|read|list|write|delete|symlink|mount|command.*defer|cleanup.*defer' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
8. `git diff --check`
|
||||
|
||||
Expected: the wire predecessor is uniquely complete; immutable request identity, exact internal namespace isolation, containment, and lifecycle tests pass under race; Darwin compilation and Node regressions pass; docs defer command/cleanup. Cached tests are not acceptable.
|
||||
|
||||
**After completing all code changes, fill implementation-owned sections in `CODE_REVIEW-*-G??.md`.**
|
||||
|
|
@ -0,0 +1,155 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/10+09_workspace_files plan=0 tag=API milestone-task=tool-executor -->
|
||||
|
||||
# Code Review Reference - API
|
||||
|
||||
> **[IMPLEMENTING AGENT — READ FIRST] Filling in this file is the mandatory final step of implementation.**
|
||||
> The task is NOT complete until every implementation-owned section below is filled in.
|
||||
> Complete the `Implementation Checklist`; the final checklist item is mandatory before saving.
|
||||
> Fill implementation-owned sections, then stop with active files in place and report ready for review.
|
||||
> Execute the plan's selected root cause, scope, files, and dependency decisions as written. Do not choose another owner, narrow/expand the write boundary, or replace a fix with another verification attempt.
|
||||
> If implementation is blocked, record the exact blocker, attempted commands/output, and resume condition only in implementation-owned evidence fields.
|
||||
> Do not ask the user directly, present choices, call user-input tools, create control-plane stop files, or classify the next state.
|
||||
> Finalization (`Code Review Result`, log rename, `complete.log`, archive moves, `Review-Only Checklist`) is review-agent-only, even after compaction/resume.
|
||||
> Follow the ownership table at the bottom of this file for which sections you own.
|
||||
|
||||
## Overview
|
||||
|
||||
date=2026-08-06
|
||||
task=m-iop-owned-single-request-agent-execution/10+09_workspace_files, plan=0, tag=API
|
||||
|
||||
## For the Review Agent
|
||||
|
||||
> **[REVIEW AGENT ONLY]** The finalization steps below are review-agent only. Implementing agents must not execute this section.
|
||||
|
||||
Compare implementation of each item against source files and verify that output in `Verification Results` matches code.
|
||||
Review completion means the following steps are finished:
|
||||
|
||||
1. Append verdict and `review_rework_count` / `evidence_integrity_failure` routing signals.
|
||||
2. Archive `CODE_REVIEW-cloud-G09.md` → `code_review_cloud_G09_0.log` and `PLAN-cloud-G08.md` → `plan_cloud_G08_0.log`.
|
||||
3. If PASS, write `complete.log` and move active task directory to `agent-task/archive/YYYY/MM/m-iop-owned-single-request-agent-execution/10+09_workspace_files/`. If WARN/FAIL, fully write the next filesystem state required by the code-review skill.
|
||||
4. If PASS, preserve the first-line `milestone-task=tool-executor` metadata in `complete.log` and report it for runtime aggregation. Roadmap state evaluation belongs to `sync-milestone-workstate`.
|
||||
5. Check applicable `Review-Only Checklist` items at the final `.log` location before reporting.
|
||||
|
||||
---
|
||||
|
||||
## Implementation Item Completion
|
||||
|
||||
| Item | Status |
|
||||
|------|---------|
|
||||
| API-1 Own immutable roots and request contexts | [ ] |
|
||||
| API-2 Execute canonical bounded file operations | [ ] |
|
||||
| API-3 Wire Node handler and bootstrap lifecycle | [ ] |
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Build a Mac-only immutable workspace catalog using `os.Root`, canonical-root checks, request identity binding, and explicit runtime lifecycle ownership.
|
||||
- [ ] Implement bounded read/list plus atomic write and non-recursive delete with fail-closed relative path, symlink, special-file, and capability validation.
|
||||
- [ ] Implement packet 09's optional Node workspace handler, bootstrap/close the runtime before ready, and keep command typed-unsupported.
|
||||
- [ ] Prove containment, bounds, concurrency, mapping, startup failure, and synchronize only implemented file-executor contract/spec claims.
|
||||
- [ ] Run dependency, focused race, package, vet, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Review-Only Checklist
|
||||
|
||||
> **[REVIEW AGENT ONLY]** Implementing agents must not modify/check this section.
|
||||
|
||||
- [ ] Append PASS/WARN/FAIL, routing signals, dimensions, and findings.
|
||||
- [ ] Archive the routed active pair to suffix `0` logs.
|
||||
- [ ] Verify managed `.gitignore` entries.
|
||||
- [ ] On PASS write `complete.log`, preserve/report Milestone metadata, move this directory, and keep the active parent while siblings remain.
|
||||
- [ ] On WARN/FAIL create only the required next loop state.
|
||||
|
||||
## Deviations from Plan
|
||||
|
||||
_Record deviations and rationale._
|
||||
|
||||
## Key Design Decisions
|
||||
|
||||
_Record implemented decisions._
|
||||
|
||||
## Reviewer Checkpoints
|
||||
|
||||
- Confirm `os.Root` is the only filesystem authority and root itself is canonical/non-symlink.
|
||||
- Confirm read/list allocation is bounded and write is same-directory atomic with no partial target.
|
||||
- Confirm escape symlinks, absolute/parent paths, special files, root delete, recursive delete, and unsupported commands fail closed.
|
||||
- Confirm bootstrap owns and closes roots before ready/reconnect teardown and errors/logs remain raw-free.
|
||||
|
||||
## Verification Results
|
||||
|
||||
Paste actual stdout/stderr for every command; record replacements under deviations.
|
||||
|
||||
### 1. Dependency
|
||||
|
||||
`test -f agent-task/m-iop-owned-single-request-agent-execution/09+08_workspace_wire/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/09+08_workspace_wire/complete.log' | wc -l)" -eq 1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 2. Runtime/file race tests
|
||||
|
||||
`go test -race ./apps/node/internal/workspace -run 'Test(Runtime|FileExecutor)' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 3. Node/bootstrap race tests
|
||||
|
||||
`go test -race ./apps/node/internal/node ./apps/node/internal/bootstrap -run 'Test(NodeWorkspace|WorkspaceRuntime)' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 4. Package regression
|
||||
|
||||
`go test ./apps/node/internal/workspace ./apps/node/internal/node ./apps/node/internal/bootstrap ./apps/node/internal/transport -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 5. Vet
|
||||
|
||||
`go vet ./apps/node/internal/workspace ./apps/node/internal/node ./apps/node/internal/bootstrap`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 6. Contract/spec search
|
||||
|
||||
`rg --sort path -n 'os.Root|read|list|write|delete|symlink|command.*defer|cleanup.*defer' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 7. Whitespace
|
||||
|
||||
`git diff --check`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
> **[IMPLEMENTING AGENT — BEFORE SAVING] Have you filled in every implementation-owned section?**
|
||||
> If anything is blank, go back and fill it in before saving this file.
|
||||
> Leave review-agent-only sections unchanged.
|
||||
|
||||
## Section Ownership
|
||||
|
||||
| Section | Owner | Note |
|
||||
|---------|-------|------|
|
||||
| Header comment, Overview, Review Agent Instructions | Fixed at stub creation | Implementing agent must not modify or execute these (archive, complete.log, and task-directory archive move are review-agent only) |
|
||||
| Archive Evidence Snapshot | Fixed at stub creation from plan when present | Implementing agent uses it as default prior-loop context; read only the specific archive files cited there when more detail is required |
|
||||
| Implementation Item Completion (item names) | Fixed at stub creation | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Implementation Checklist (item text/order) | Fixed at stub creation from plan | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Review-Only Checklist | Review agent only | Implementing agent must not modify or check this section |
|
||||
| Deviations from Plan, Key Design Decisions | Implementing agent | Replace placeholder text with actual content |
|
||||
| Reviewer Checkpoints | Fixed at stub creation | Pre-filled from plan |
|
||||
| Verification Results (section headings + commands) | Fixed at stub creation | Implementing agent fills in command output only; command changes require a `Deviations from Plan` entry |
|
||||
| Code Review Result | Review agent appends | Not included in stub |
|
||||
|
|
@ -0,0 +1,250 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/10+09_workspace_files plan=0 tag=API milestone-task=tool-executor -->
|
||||
|
||||
# Mac Workspace File Executor
|
||||
|
||||
## For the Implementing Agent
|
||||
|
||||
Do not start until packet 09 has `complete.log`. Implement only the exact files listed, preserve the typed wire contract, run every verification command, and fill `CODE_REVIEW-cloud-G09.md`. Official review owns verdict, logs, completion, and archive moves.
|
||||
|
||||
## Background
|
||||
|
||||
The dedicated wire is intentionally inert until Node can validate its private catalog, open a request context, and execute canonical read/list/write/delete operations beneath one root. This packet supplies that Mac-owned filesystem boundary and leaves command/process execution to packet 11.
|
||||
|
||||
## Analysis
|
||||
|
||||
### Files Read
|
||||
|
||||
- `AGENTS.md`
|
||||
- `agent-ops/rules/project/rules.md`
|
||||
- `agent-ops/rules/common/rules-roadmap.md`
|
||||
- `agent-ops/rules/common/rules-agent-spec.md`
|
||||
- `agent-ops/rules/project/domain/node/rules.md`
|
||||
- `agent-ops/rules/project/domain/platform-common/rules.md`
|
||||
- `agent-test/local/rules.md`
|
||||
- `agent-test/local/node-smoke.md`
|
||||
- `agent-roadmap/phase/knowledge-tool-optimization-extension/milestones/iop-owned-single-request-agent-execution.md`
|
||||
- `agent-roadmap/sdd/knowledge-tool-optimization-extension/iop-owned-single-request-agent-execution/SDD.md`
|
||||
- `apps/node/internal/bootstrap/module.go`
|
||||
- `apps/node/internal/node/node.go`
|
||||
- `apps/node/internal/node/config_refresh_handler.go`
|
||||
- `apps/node/internal/transport/session.go`
|
||||
- `apps/edge/internal/node/mapper.go`
|
||||
- `proto/iop/runtime.proto`
|
||||
- `agent-contract/inner/edge-node-runtime-wire.md`
|
||||
- `agent-contract/inner/execution-runtime.md`
|
||||
- `agent-spec/runtime/edge-node-execution.md`
|
||||
|
||||
### SDD Criteria
|
||||
|
||||
- S04 requires approved root containment and rejection of absolute/foreign paths and symlink escape before effects.
|
||||
- S05 requires typed read/list/write/delete success/failure and bounded output. D06 excludes interactive shell/desktop/scheduler.
|
||||
- Cleanup and command process groups remain packets 13 and 11 respectively.
|
||||
|
||||
### Verification Context
|
||||
|
||||
- Host Go is 1.26.2; module baseline supports Go 1.24, whose `os.Root` provides root-relative APIs and rejects symlink traversal outside the root.
|
||||
- `go doc os.Root` confirms methods are concurrent-safe and that special files/mounts still require explicit rejection.
|
||||
- Baseline Node/bootstrap/transport packages passed fresh. Filesystem tests use `t.TempDir`; production Mac platform validation is injected with an explicit host-OS argument for deterministic Linux CI tests.
|
||||
|
||||
### State and Concurrency Findings
|
||||
|
||||
- One runtime catalog owns long-lived `os.Root` handles; request contexts bind execution id to exactly one ref and immutable caps.
|
||||
- Duplicate open is idempotent only for byte-identical binding; conflicting reuse fails. A request cannot operate before open or after close/cleanup.
|
||||
- `os.Root` supplies containment, but executor must separately reject empty/absolute/parent paths, root deletion, non-regular reads/writes, devices/FIFOs, recursive user delete, and unbounded reads/lists.
|
||||
|
||||
### Test Coverage Gaps
|
||||
|
||||
- Node has no workspace runtime field/handler or bootstrap catalog validation.
|
||||
- No test covers in-root symlinks versus escaping symlinks, special files, bounded listing/read, atomic writes, or concurrent request isolation.
|
||||
|
||||
### Symbol References
|
||||
|
||||
- Packet 09's optional `transport.WorkspaceHandler` is implemented by `*node.Node`; existing `transport.Handler` remains unchanged.
|
||||
- `runtimeOwner.close` must close workspace roots during reconnect/shutdown.
|
||||
- No provider adapter/router/store API is reused.
|
||||
|
||||
### Split Judgment
|
||||
|
||||
- Stable contract: file-only runtime independently passes every S04 containment case and returns typed unsupported for command.
|
||||
- Command is separate because process-group cancellation and environment/output races are a distinct correctness boundary.
|
||||
|
||||
### Scope Rationale
|
||||
|
||||
- Include catalog startup validation, request contexts, file operations, Node handler, bootstrap lifecycle, tests, contract/spec sync.
|
||||
- Exclude command execution, provider tool loop, cleanup artifact removal, and metrics.
|
||||
|
||||
### Final Routing
|
||||
|
||||
- `evaluation_mode=first-pass`; build closures true, scores 2/1/2/1/2 = G08.
|
||||
- Finalizer route `risk-boundary`, lane `cloud`, filename `PLAN-cloud-G08.md`; risks `boundary_contract`, `structured_interpretation`, `variant_product`, `concurrent_consistency` (4).
|
||||
- Review scores 2/1/2/2/2 = G09; official filename `CODE_REVIEW-cloud-G09.md`; no capability gap or recovery signal.
|
||||
|
||||
## Dependencies and Execution Order
|
||||
|
||||
1. Require packet 09 completion.
|
||||
2. Implement the private catalog/root layer and file operations.
|
||||
3. Adapt typed messages in Node, then wire runtime ownership in bootstrap.
|
||||
4. Run race/containment tests before updating contract/spec claims.
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Build a Mac-only immutable workspace catalog using `os.Root`, canonical-root checks, request identity binding, and explicit runtime lifecycle ownership.
|
||||
- [ ] Implement bounded read/list plus atomic write and non-recursive delete with fail-closed relative path, symlink, special-file, and capability validation.
|
||||
- [ ] Implement packet 09's optional Node workspace handler, bootstrap/close the runtime before ready, and keep command typed-unsupported.
|
||||
- [ ] Prove containment, bounds, concurrency, mapping, startup failure, and synchronize only implemented file-executor contract/spec claims.
|
||||
- [ ] Run dependency, focused race, package, vet, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Implementation Plan
|
||||
|
||||
### [API-1] Own immutable roots and request contexts
|
||||
|
||||
**Problem**
|
||||
|
||||
- `apps/node/internal/node/node.go:18` has only provider runtime fields.
|
||||
- `apps/node/internal/bootstrap/module.go:100` builds adapters immediately from registration config and signals ready without validating workspaces.
|
||||
|
||||
**Solution**
|
||||
|
||||
Before (`apps/node/internal/node/node.go:18`):
|
||||
|
||||
```go
|
||||
type Node struct {
|
||||
nodeID string
|
||||
router runtime.Router
|
||||
store *store.Store
|
||||
// provider fields
|
||||
}
|
||||
```
|
||||
|
||||
After:
|
||||
|
||||
```go
|
||||
type Node struct {
|
||||
// Existing fields remain.
|
||||
workspaceRuntime *workspace.Runtime
|
||||
}
|
||||
|
||||
func (n *Node) SetWorkspaceRuntime(rt *workspace.Runtime) { n.workspaceRuntime = rt }
|
||||
```
|
||||
|
||||
Create `apps/node/internal/workspace` with an immutable catalog. Constructor accepts packet 09 configs plus explicit host OS, requires `darwin`, validates the configured root exists, is an absolute directory and not a symlink/root, and opens `os.Root`. `Open` binds a validated execution id to one workspace ref and copied lower limits; conflicting duplicate ids fail. `Close` makes all roots unavailable and is idempotent.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/node/internal/workspace/runtime.go` — catalog, root handles, request map, open/close, identity and capability checks.
|
||||
- [ ] `apps/node/internal/workspace/runtime_test.go` — platform/root/startup, duplicate/open/close, copy and concurrent request isolation.
|
||||
- [ ] `apps/node/internal/node/node.go` — hold the optional runtime without changing constructor call sites.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Test valid injected `darwin`, wrong host OS, root symlink, missing/non-directory/root path, duplicate refs, conflicting execution id, and concurrent isolation under `-race`.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/node/internal/workspace -run 'TestRuntime(Open|Catalog|Close|Concurrent)' -count=1`
|
||||
- Expected: only a validated Mac catalog opens and request identities never cross roots.
|
||||
|
||||
### [API-2] Execute canonical bounded file operations
|
||||
|
||||
**Problem**
|
||||
|
||||
- No Node code executes relative workspace tools; using `os.ReadFile`/ordinary joins would allow unbounded allocation or TOCTOU escape.
|
||||
|
||||
**Solution**
|
||||
|
||||
Add path validation before every operation and use only the request's `*os.Root`. Read through `io.LimitReader(max+1)`, stat the opened handle as regular, and return typed truncation/error. List a directory with stable lexical ordering, entry/type encoding, and shared byte/entry bound. Write rejects oversized input and non-regular existing targets, creates parents through `Root.MkdirAll`, writes/fsyncs a random same-directory temp, and atomically renames. Delete rejects `.` and removes only a regular file, symlink itself, or empty directory; never recursively deletes user paths. Command returns the packet 09 unsupported error until packet 11.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/node/internal/workspace/path.go` — canonical relative path and reserved-root validation.
|
||||
- [ ] `apps/node/internal/workspace/file_executor.go` — bounded read/list, atomic write, non-recursive delete, typed results.
|
||||
- [ ] `apps/node/internal/workspace/file_executor_test.go` — operation/capability matrix, bounds, symlinks, special files, atomic replacement, and concurrent roots.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Use `t.TempDir`, inside/outside symlinks, FIFO where supported, large fixtures, and parallel operations. Verify no outside file changes and no partial target after failed write.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/node/internal/workspace -run 'TestFileExecutor' -count=1`
|
||||
- Expected: all canonical operations work inside root and every escape/special/bound violation fails before outside or partial effects.
|
||||
|
||||
### [API-3] Wire Node handler and bootstrap lifecycle
|
||||
|
||||
**Problem**
|
||||
|
||||
- `apps/node/internal/bootstrap/module.go:124` creates Node and signals ready without workspace initialization.
|
||||
- Packet 09's optional handler currently returns unsupported because `Node` does not implement it.
|
||||
|
||||
**Solution**
|
||||
|
||||
Before (`apps/node/internal/bootstrap/module.go:124`):
|
||||
|
||||
```go
|
||||
rtr := router.New(set.Registry, logger)
|
||||
n := node.New(result.NodeID, rtr, st, globalConcurrency, os.Stdout, logger, set)
|
||||
```
|
||||
|
||||
After:
|
||||
|
||||
```go
|
||||
workspaceRuntime, err := workspace.NewRuntime(result.Config.GetWorkspaces(), runtime.GOOS, logger)
|
||||
if err != nil { /* close owner and fail before ready */ }
|
||||
n := node.New(/* existing args */)
|
||||
n.SetWorkspaceRuntime(workspaceRuntime)
|
||||
owner.workspace = workspaceRuntime
|
||||
```
|
||||
|
||||
Implement open/tool dispatch in `workspace_handler.go`, echo identities, map only stable error codes/messages, and never log request path/content/result. Cancel/cleanup remain typed unsupported. Extend `runtimeOwner.close` to close the workspace runtime before session/store teardown.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/node/internal/node/workspace_handler.go` — implement open/file tool responses and typed unsupported command/cancel/cleanup.
|
||||
- [ ] `apps/node/internal/node/workspace_handler_test.go` — handler mapping, missing runtime, identity, raw-free error cases.
|
||||
- [ ] `apps/node/internal/bootstrap/module.go` — construct/own/close runtime before `SignalReady`.
|
||||
- [ ] `apps/node/internal/bootstrap/workspace_runtime_test.go` — startup success/failure and close ownership with injected payload/host OS helper.
|
||||
- [ ] `agent-contract/inner/edge-node-runtime-wire.md` — mark file semantics implemented and command/cleanup deferred.
|
||||
- [ ] `agent-spec/runtime/edge-node-execution.md` — synchronize current runtime entry points and tests.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Use direct handler tests and a bootstrap composition helper; no real Edge or Mac host is required.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/node/internal/node ./apps/node/internal/bootstrap -run 'Test(NodeWorkspace|WorkspaceRuntime)' -count=1`
|
||||
- `rg --sort path -n 'os.Root|read|list|write|delete|symlink|command.*defer|cleanup.*defer' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
- Expected: Node becomes ready only after valid root ownership and docs claim file operations only.
|
||||
|
||||
## Modified Files Summary
|
||||
|
||||
| File | Item |
|
||||
|------|------|
|
||||
| `apps/node/internal/workspace/runtime.go` | API-1 |
|
||||
| `apps/node/internal/workspace/runtime_test.go` | API-1 |
|
||||
| `apps/node/internal/node/node.go` | API-1 |
|
||||
| `apps/node/internal/workspace/path.go` | API-2 |
|
||||
| `apps/node/internal/workspace/file_executor.go` | API-2 |
|
||||
| `apps/node/internal/workspace/file_executor_test.go` | API-2 |
|
||||
| `apps/node/internal/node/workspace_handler.go` | API-3 |
|
||||
| `apps/node/internal/node/workspace_handler_test.go` | API-3 |
|
||||
| `apps/node/internal/bootstrap/module.go` | API-3 |
|
||||
| `apps/node/internal/bootstrap/workspace_runtime_test.go` | API-3 |
|
||||
| `agent-contract/inner/edge-node-runtime-wire.md` | API-3 |
|
||||
| `agent-spec/runtime/edge-node-execution.md` | API-3 |
|
||||
| `agent-task/m-iop-owned-single-request-agent-execution/10+09_workspace_files/CODE_REVIEW-cloud-G09.md` | API-1, API-2, API-3 |
|
||||
|
||||
## Final Verification
|
||||
|
||||
1. `test -f agent-task/m-iop-owned-single-request-agent-execution/09+08_workspace_wire/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/09+08_workspace_wire/complete.log' | wc -l)" -eq 1`
|
||||
2. `go test -race ./apps/node/internal/workspace -run 'Test(Runtime|FileExecutor)' -count=1`
|
||||
3. `go test -race ./apps/node/internal/node ./apps/node/internal/bootstrap -run 'Test(NodeWorkspace|WorkspaceRuntime)' -count=1`
|
||||
4. `go test ./apps/node/internal/workspace ./apps/node/internal/node ./apps/node/internal/bootstrap ./apps/node/internal/transport -count=1`
|
||||
5. `go vet ./apps/node/internal/workspace ./apps/node/internal/node ./apps/node/internal/bootstrap`
|
||||
6. `rg --sort path -n 'os.Root|read|list|write|delete|symlink|command.*defer|cleanup.*defer' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
7. `git diff --check`
|
||||
|
||||
Expected: the wire predecessor is uniquely complete; all containment and lifecycle tests pass under race; Node regressions pass; docs defer command/cleanup. Cached tests are not acceptable.
|
||||
|
||||
**After completing all code changes, fill implementation-owned sections in `CODE_REVIEW-*-G??.md`.**
|
||||
|
|
@ -0,0 +1,167 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/11+10_workspace_command plan=1 tag=API milestone-task=tool-executor -->
|
||||
|
||||
# Code Review Reference - API
|
||||
|
||||
> **[IMPLEMENTING AGENT — READ FIRST] Filling in this file is the mandatory final step of implementation.**
|
||||
> The task is NOT complete until every implementation-owned section below is filled in.
|
||||
> Complete the `Implementation Checklist`; the final checklist item is mandatory before saving.
|
||||
> Fill implementation-owned sections, then stop with active files in place and report ready for review.
|
||||
> Execute the plan's selected root cause, scope, files, and dependency decisions as written. Do not choose another owner, narrow/expand the write boundary, or replace a fix with another verification attempt.
|
||||
> If implementation is blocked, record the exact blocker, attempted commands/output, and resume condition only in implementation-owned evidence fields.
|
||||
> Do not ask the user directly, present choices, call user-input tools, create control-plane stop files, or classify the next state.
|
||||
> Finalization (`Code Review Result`, log rename, `complete.log`, archive moves, `Review-Only Checklist`) is review-agent-only, even after compaction/resume.
|
||||
> Follow the ownership table at the bottom of this file for which sections you own.
|
||||
|
||||
## Overview
|
||||
|
||||
date=2026-08-06
|
||||
task=m-iop-owned-single-request-agent-execution/11+10_workspace_command, plan=1, tag=API
|
||||
|
||||
## Archive Evidence Snapshot
|
||||
|
||||
- The first-pass pair is preserved at `agent-task/m-iop-owned-single-request-agent-execution/11+10_workspace_command/plan_cloud_G08_0.log` and `agent-task/m-iop-owned-single-request-agent-execution/11+10_workspace_command/code_review_cloud_G10_0.log`; it contains no implementation evidence or review verdict.
|
||||
- Self-review found that assigning `cmd.Dir` to the configured path re-resolves that path at process start and can leave the admitted workspace after a rename/replacement. Plan 1 requires an internal child-launch shim to `fchdir` packet 10's opened root descriptor before executing the fixed template and fails before target start when identity cannot be preserved.
|
||||
|
||||
## For the Review Agent
|
||||
|
||||
> **[REVIEW AGENT ONLY]** The finalization steps below are review-agent only. Implementing agents must not execute this section.
|
||||
|
||||
Compare implementation of each item against source files and verify that output in `Verification Results` matches code.
|
||||
Review completion means the following steps are finished:
|
||||
|
||||
1. Append verdict and `review_rework_count` / `evidence_integrity_failure` routing signals.
|
||||
2. Archive `CODE_REVIEW-cloud-G10.md` → `code_review_cloud_G10_1.log` and `PLAN-cloud-G09.md` → `plan_cloud_G09_1.log`.
|
||||
3. If PASS, write `complete.log` and move active task directory to `agent-task/archive/YYYY/MM/m-iop-owned-single-request-agent-execution/11+10_workspace_command/`. If WARN/FAIL, fully write the next filesystem state required by the code-review skill.
|
||||
4. If PASS, preserve the first-line `milestone-task=tool-executor` metadata in `complete.log` and report it for runtime aggregation. Roadmap state evaluation belongs to `sync-milestone-workstate`.
|
||||
5. Check applicable `Review-Only Checklist` items at the final `.log` location before reporting.
|
||||
|
||||
---
|
||||
|
||||
## Implementation Item Completion
|
||||
|
||||
| Item | Status |
|
||||
|------|---------|
|
||||
| API-1 Implement exact-template process execution | [ ] |
|
||||
| API-2 Activate typed command and cancel handling | [ ] |
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Resolve only operator-defined command ids to absolute executable/fixed args, enter the opened admitted root with an internal `fchdir`/`exec` shim, and build a minimal allowlisted environment.
|
||||
- [ ] Own Unix process groups with one terminal result across exit, timeout, context cancel, explicit cancel, and shared stdout/stderr truncation races.
|
||||
- [ ] Integrate command/cancel into the workspace runtime and Node handler without touching provider cancellation or permitting shell/PTY/arbitrary argv.
|
||||
- [ ] Prove success/nonzero/timeout/cancel/group-child/output/env/cross-request behavior plus root rename/replacement resistance, and synchronize command contract/spec limits.
|
||||
- [ ] Run dependency, focused race, package, vet, cross-build, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Review-Only Checklist
|
||||
|
||||
> **[REVIEW AGENT ONLY]** Implementers must not modify/check this section.
|
||||
|
||||
- [ ] Append verdict, routing signals, dimensions, and findings.
|
||||
- [ ] Archive the active pair to routed suffix `1` logs.
|
||||
- [ ] Verify managed `.gitignore` entries.
|
||||
- [ ] On PASS write `complete.log`, preserve/report Milestone metadata, move the directory, and keep the active parent while siblings remain.
|
||||
- [ ] On WARN/FAIL write only the required next state.
|
||||
|
||||
## Deviations from Plan
|
||||
|
||||
_Record deviations and rationale._
|
||||
|
||||
## Key Design Decisions
|
||||
|
||||
_Record implemented decisions._
|
||||
|
||||
## Reviewer Checkpoints
|
||||
|
||||
- Confirm executable and args come only from the approved template; caller supplies no shell/arbitrary argv.
|
||||
- Confirm the internal shim validates the opened admitted directory descriptor, calls `fchdir`, then replaces itself with only the fixed target; path rename/replacement cannot redirect it, malformed control cannot start a target, and no ambient secret is inherited.
|
||||
- Confirm one wait/result owner and entire process-group termination for every cancel/timeout race.
|
||||
- Confirm stdout/stderr share a cap while overflow drains, and cross-request cancel cannot kill another group.
|
||||
|
||||
## Verification Results
|
||||
|
||||
Paste actual stdout/stderr for each command; record replacements under deviations.
|
||||
|
||||
### 1. Dependency
|
||||
|
||||
`test -f agent-task/m-iop-owned-single-request-agent-execution/10+09_workspace_files/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/10+09_workspace_files/complete.log' | wc -l)" -eq 1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 2. Process race tests
|
||||
|
||||
`go test -race ./apps/node/internal/workspace -run 'Test(CommandExecutor|WorkspaceCommandHelperProcess)' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 3. Handler race tests
|
||||
|
||||
`go test -race ./apps/node/internal/node -run 'TestNodeWorkspace(Command|Cancel)' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 4. Package regression
|
||||
|
||||
`go test ./apps/node/internal/workspace ./apps/node/internal/node ./apps/node/internal/transport ./apps/node/cmd/node -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 5. Vet
|
||||
|
||||
`go vet ./apps/node/internal/workspace ./apps/node/internal/node ./apps/node/cmd/node`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 6. Darwin compile
|
||||
|
||||
`GOOS=darwin GOARCH=arm64 go test -c -o /tmp/iop-workspace-command-darwin.test ./apps/node/internal/workspace`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 7. Contract/spec search
|
||||
|
||||
`rg --sort path -n 'command id|fixed args|fchdir|exec|cwd|process group|environment allowlist|stdout|stderr|PTY|shell' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 8. Whitespace
|
||||
|
||||
`git diff --check`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
> **[IMPLEMENTING AGENT — BEFORE SAVING] Have you filled in every implementation-owned section?**
|
||||
> If anything is blank, go back and fill it in before saving this file.
|
||||
> Leave review-agent-only sections unchanged.
|
||||
|
||||
## Section Ownership
|
||||
|
||||
| Section | Owner | Note |
|
||||
|---------|-------|------|
|
||||
| Header comment, Overview, Review Agent Instructions | Fixed at stub creation | Implementing agent must not modify or execute these (archive, complete.log, and task-directory archive move are review-agent only) |
|
||||
| Archive Evidence Snapshot | Fixed at stub creation from plan when present | Implementing agent uses it as default prior-loop context; read only the specific archive files cited there when more detail is required |
|
||||
| Implementation Item Completion (item names) | Fixed at stub creation | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Implementation Checklist (item text/order) | Fixed at stub creation from plan | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Review-Only Checklist | Review agent only | Implementing agent must not modify or check this section |
|
||||
| Deviations from Plan, Key Design Decisions | Implementing agent | Replace placeholder text with actual content |
|
||||
| Reviewer Checkpoints | Fixed at stub creation | Pre-filled from plan |
|
||||
| Verification Results (section headings + commands) | Fixed at stub creation | Implementing agent fills in command output only; command changes require a `Deviations from Plan` entry |
|
||||
| Code Review Result | Review agent appends | Not included in stub |
|
||||
|
|
@ -0,0 +1,216 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/11+10_workspace_command plan=1 tag=API milestone-task=tool-executor -->
|
||||
|
||||
# Bounded Workspace Command Executor
|
||||
|
||||
## For the Implementing Agent
|
||||
|
||||
Do not start until packet 10 has `complete.log`. Use only operator-owned exact command templates, implement within the listed boundary, run every verification command, and fill `CODE_REVIEW-cloud-G10.md`. Do not introduce shell/PTY/general argv execution or own review finalization.
|
||||
|
||||
## Background
|
||||
|
||||
Packet 10 deliberately returns typed unsupported for command. This packet activates only exact operator-configured command ids in the opened admitted workspace cwd, with environment allowlisting, shared output bounds, deadline, process-group cancellation, and race-safe result ownership.
|
||||
|
||||
## Archive Evidence Snapshot
|
||||
|
||||
- The first-pass pair is preserved at `agent-task/m-iop-owned-single-request-agent-execution/11+10_workspace_command/plan_cloud_G08_0.log` and `agent-task/m-iop-owned-single-request-agent-execution/11+10_workspace_command/code_review_cloud_G10_0.log`; it contains no implementation evidence or review verdict.
|
||||
- Self-review found that assigning `cmd.Dir` to the configured path re-resolves that path at process start and can leave the admitted workspace after a rename/replacement. Plan 1 requires an internal child-launch shim to `fchdir` packet 10's opened root descriptor before executing the fixed template and fails before target start when identity cannot be preserved.
|
||||
|
||||
## Analysis
|
||||
|
||||
### Files Read
|
||||
|
||||
- `AGENTS.md`
|
||||
- `agent-ops/rules/project/rules.md`
|
||||
- `agent-ops/rules/common/rules-roadmap.md`
|
||||
- `agent-ops/rules/common/rules-agent-spec.md`
|
||||
- `agent-ops/rules/project/domain/node/rules.md`
|
||||
- `agent-ops/rules/project/domain/platform-common/rules.md`
|
||||
- `agent-test/local/rules.md`
|
||||
- `agent-test/local/node-smoke.md`
|
||||
- `agent-roadmap/phase/knowledge-tool-optimization-extension/milestones/iop-owned-single-request-agent-execution.md`
|
||||
- `agent-roadmap/sdd/knowledge-tool-optimization-extension/iop-owned-single-request-agent-execution/SDD.md`
|
||||
- `apps/node/internal/node/node.go`
|
||||
- `apps/node/internal/node/cancel_handler.go`
|
||||
- `apps/node/internal/node/run_handler.go`
|
||||
- `apps/node/internal/transport/session.go`
|
||||
- `packages/go/config/edge_types.go`
|
||||
- `agent-contract/inner/edge-node-runtime-wire.md`
|
||||
- `agent-contract/inner/execution-runtime.md`
|
||||
- `agent-spec/runtime/edge-node-execution.md`
|
||||
|
||||
### SDD Criteria
|
||||
|
||||
- S05 requires command success/failure/timeout/large output plus process cancel and consistent typed results.
|
||||
- D06 excludes interactive terminal, shell, desktop, scheduler, and long-lived agent processes.
|
||||
- Cwd must be the opened admitted root identity rather than a later pathname lookup; process group, output cap, timeout, and environment allowlist are mandatory and fail closed.
|
||||
|
||||
### Verification Context
|
||||
|
||||
- Current source has no Node subprocess path; the only `exec.Command` use is host setup, so there is no compatible executor to extend.
|
||||
- Target is Mac, while CI host is Linux. Unix process-group implementation must be build-tagged for Darwin/Linux and tested on Linux; an unsupported fallback keeps other builds explicit.
|
||||
- Tests use the Go test binary as an exact configured executable, not `/bin/sh`.
|
||||
- Packet 10 retains an opened directory handle for the admitted root. Each command duplicates and inherits that handle into a short-lived internal launch shim, which verifies the directory identity, calls `fchdir`, and replaces itself with the fixed target executable. It never resolves the configured root string again.
|
||||
|
||||
### State and Concurrency Findings
|
||||
|
||||
- One tool call owns one process group and one terminal result; timeout, explicit cancel, context cancel, exit, and output overflow race through a single completion path.
|
||||
- Output writers must share one total cap and continue draining after truncation so child pipes cannot deadlock.
|
||||
- Cancel addresses only `(request_id, tool_call_id)` and cannot kill another request's process.
|
||||
|
||||
### Test Coverage Gaps
|
||||
|
||||
- No exact-template command lookup, minimal environment builder, process group owner, capped writer, or cancel race exists.
|
||||
|
||||
### Symbol References
|
||||
|
||||
- Packet 07 defines command templates; packet 09 defines command/cancel messages; packet 10 owns the runtime and Node handler.
|
||||
- Do not reuse provider `runManager`, `OnCancel`, `exec.CommandContext`'s single-process kill, or any caller shell codec.
|
||||
|
||||
### Split Judgment
|
||||
|
||||
- Command/process correctness is one indivisible slice: start, output drain, timeout/cancel group kill, and wait/result ownership must be reviewed together.
|
||||
- Cleanup of request artifacts and all processes remains packet 13, which builds on this per-tool primitive.
|
||||
|
||||
### Scope Rationale
|
||||
|
||||
- Include exact command id execution, cwd/env/bounds, Unix group lifecycle, typed cancel/result, tests, contract/spec.
|
||||
- Exclude arbitrary argv/shell, PTY, network sandbox claims, cleanup orchestration, provider loop, and public output.
|
||||
|
||||
### Final Routing
|
||||
|
||||
- `evaluation_mode=isolated-reassessment`; `review_rework_count=0`; `evidence_integrity_failure=false`; build closures true, scores 2/2/2/1/2 = G09.
|
||||
- Finalizer route `grade-boundary`, lane `cloud`, filename `PLAN-cloud-G09.md`; risks `temporal_state`, `concurrent_consistency`, `boundary_contract`, `variant_product` (4).
|
||||
- Review scores 2/2/2/2/2 = G10; official filename `CODE_REVIEW-cloud-G10.md`; no recovery/capability gap.
|
||||
|
||||
## Dependencies and Execution Order
|
||||
|
||||
1. Require packet 10 completion.
|
||||
2. Implement capped output and process-group lifecycle before runtime dispatch.
|
||||
3. Wire command and cancel through the existing workspace handler, then update docs.
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Resolve only operator-defined command ids to absolute executable/fixed args, enter the opened admitted root with an internal `fchdir`/`exec` shim, and build a minimal allowlisted environment.
|
||||
- [ ] Own Unix process groups with one terminal result across exit, timeout, context cancel, explicit cancel, and shared stdout/stderr truncation races.
|
||||
- [ ] Integrate command/cancel into the workspace runtime and Node handler without touching provider cancellation or permitting shell/PTY/arbitrary argv.
|
||||
- [ ] Prove success/nonzero/timeout/cancel/group-child/output/env/cross-request behavior plus root rename/replacement resistance, and synchronize command contract/spec limits.
|
||||
- [ ] Run dependency, focused race, package, vet, cross-build, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Implementation Plan
|
||||
|
||||
### [API-1] Implement exact-template process execution
|
||||
|
||||
**Problem**
|
||||
|
||||
- Packet 10's planned `file_executor.go` returns unsupported for command; no process owner or bounded writer exists.
|
||||
- `apps/node/internal/node/cancel_handler.go:12` cancels provider runs and must not be overloaded with workspace process identity.
|
||||
|
||||
**Solution**
|
||||
|
||||
No command executor exists. Add:
|
||||
|
||||
```go
|
||||
type commandExecution struct {
|
||||
done chan struct{}
|
||||
cancelOnce sync.Once
|
||||
// process/result state guarded by one owner
|
||||
}
|
||||
|
||||
func (r *Runtime) executeCommand(ctx context.Context, request Request, input CommandInput) Result
|
||||
func (r *Runtime) Cancel(requestID, toolCallID string) CancelResult
|
||||
```
|
||||
|
||||
Resolve `command_id` to the immutable config template and run its absolute executable plus fixed args only. Reject request argv, unknown ids, disabled capability, unapproved env names/invalid values, and timeout/output bounds. Duplicate packet 10's opened admitted directory handle into a fixed inherited fd and launch only the current trusted Node/test executable in an internal shim mode. Transfer a bounded, versioned launch record over inherited pipes; it is assembled solely from the immutable command template and validated environment, never caller argv. In the Unix shim, `fstat` the inherited directory fd against the admitted device/inode, call `fchdir`, close control fds, and `unix.Exec` the configured absolute executable/fixed args with the explicit minimal target environment. The exec replacement preserves the shim's process-group identity. Report a closed pre-exec error code to the parent if record validation, identity, `fchdir`, or `exec` fails; do not start the target on those paths. Never use `cmd.Dir`, a descriptor pathname, a shell, or a re-opened configured root. Route stdout/stderr through one concurrency-safe total byte budget, retain separate bounded streams, mark truncation, and continue discarding overflow.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/node/internal/workspace/command_executor.go` — lookup, minimal env, start/wait/result arbitration, shared capped output.
|
||||
- [ ] `apps/node/internal/workspace/command_process_unix.go` — Darwin/Linux inherited-fd launch record, `fstat`/`fchdir`/`exec`, process group creation, and group signal/kill.
|
||||
- [ ] `apps/node/internal/workspace/command_process_other.go` — explicit unsupported fallback for non-Unix builds.
|
||||
- [ ] `apps/node/internal/workspace/command_executor_test.go` — helper-process success, exit, descriptor cwd/env, root rename/replacement, timeout, cancel, child group, output, and request isolation.
|
||||
- [ ] `apps/node/cmd/node/main.go` — enter the internal workspace launch shim before Cobra parsing; normal CLI behavior remains unchanged.
|
||||
- [ ] `apps/node/cmd/node/main_test.go` — prove absent/malformed shim control cannot execute a target and normal commands remain compatible.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Use `os.Executable()` plus `-test.run=TestWorkspaceCommandHelperProcess` as the exact configured target. `TestMain` enters the same internal shim mode used by the Node binary, and the helper emits stdout/stderr, spawns a child, blocks, exits non-zero, and records cwd/env as directed. Open a workspace, rename its configured root and replace the old path with a foreign directory/symlink before command start, then assert the target either runs in the originally admitted directory identity or never starts; it must never enter the replacement. Corrupt the inherited record/fd identity and assert a closed pre-exec failure with no target sentinel.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/node/internal/workspace -run 'Test(CommandExecutor|WorkspaceCommandHelperProcess)' -count=1`
|
||||
- Expected: one typed outcome wins every race, descendants die, retained output never exceeds the shared cap, and path replacement cannot redirect cwd.
|
||||
|
||||
### [API-2] Activate typed command and cancel handling
|
||||
|
||||
**Problem**
|
||||
|
||||
- Packet 10 leaves `WorkspaceToolOperation_COMMAND` and `WorkspaceCancelRequest` unsupported at the Node handler boundary.
|
||||
|
||||
**Solution**
|
||||
|
||||
Before (packet 10 contract):
|
||||
|
||||
```go
|
||||
case iop.WORKSPACE_TOOL_OPERATION_COMMAND:
|
||||
return unsupportedResult(req)
|
||||
```
|
||||
|
||||
After:
|
||||
|
||||
```go
|
||||
case iop.WORKSPACE_TOOL_OPERATION_COMMAND:
|
||||
return n.workspaceRuntime.Execute(ctx, decodeCommand(req))
|
||||
```
|
||||
|
||||
Decode only command id/env/timeout/output cap; validate identity before dispatch. Implement `OnWorkspaceCancel` through the workspace runtime, make duplicate cancel idempotent, and return typed not-found without touching another execution. Keep cleanup unsupported.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/node/internal/workspace/runtime.go` — track active commands by exact request/tool identity and expose race-safe cancel.
|
||||
- [ ] `apps/node/internal/node/workspace_handler.go` — decode command and map cancel/result status.
|
||||
- [ ] `apps/node/internal/node/workspace_handler_test.go` — command/cancel mapping, duplicate/not-found, and raw-free errors.
|
||||
- [ ] `agent-contract/inner/edge-node-runtime-wire.md` — define exact-template trust boundary, stable descriptor cwd/env/output/process semantics, and exclusions.
|
||||
- [ ] `agent-spec/runtime/edge-node-execution.md` — mark command/cancel implemented with named tests.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Extend direct Node handler tests and assert raw sentinels never appear in logged/typed error text.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/node/internal/node -run 'TestNodeWorkspace(Command|Cancel)' -count=1`
|
||||
- `rg --sort path -n 'command id|fixed args|process group|environment allowlist|stdout|stderr|PTY|shell' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
- Expected: typed command/cancel is active and the fixed-template/non-interactive boundary is explicit.
|
||||
|
||||
## Modified Files Summary
|
||||
|
||||
| File | Item |
|
||||
|------|------|
|
||||
| `apps/node/internal/workspace/command_executor.go` | API-1 |
|
||||
| `apps/node/internal/workspace/command_process_unix.go` | API-1 |
|
||||
| `apps/node/internal/workspace/command_process_other.go` | API-1 |
|
||||
| `apps/node/internal/workspace/command_executor_test.go` | API-1 |
|
||||
| `apps/node/cmd/node/main.go` | API-1 |
|
||||
| `apps/node/cmd/node/main_test.go` | API-1 |
|
||||
| `apps/node/internal/workspace/runtime.go` | API-2 |
|
||||
| `apps/node/internal/node/workspace_handler.go` | API-2 |
|
||||
| `apps/node/internal/node/workspace_handler_test.go` | API-2 |
|
||||
| `agent-contract/inner/edge-node-runtime-wire.md` | API-2 |
|
||||
| `agent-spec/runtime/edge-node-execution.md` | API-2 |
|
||||
| `agent-task/m-iop-owned-single-request-agent-execution/11+10_workspace_command/CODE_REVIEW-cloud-G10.md` | API-1, API-2 |
|
||||
|
||||
## Final Verification
|
||||
|
||||
1. `test -f agent-task/m-iop-owned-single-request-agent-execution/10+09_workspace_files/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/10+09_workspace_files/complete.log' | wc -l)" -eq 1`
|
||||
2. `go test -race ./apps/node/internal/workspace -run 'Test(CommandExecutor|WorkspaceCommandHelperProcess)' -count=1`
|
||||
3. `go test -race ./apps/node/internal/node -run 'TestNodeWorkspace(Command|Cancel)' -count=1`
|
||||
4. `go test ./apps/node/internal/workspace ./apps/node/internal/node ./apps/node/internal/transport ./apps/node/cmd/node -count=1`
|
||||
5. `go vet ./apps/node/internal/workspace ./apps/node/internal/node ./apps/node/cmd/node`
|
||||
6. `GOOS=darwin GOARCH=arm64 go test -c -o /tmp/iop-workspace-command-darwin.test ./apps/node/internal/workspace`
|
||||
7. `rg --sort path -n 'command id|fixed args|fchdir|exec|cwd|process group|environment allowlist|stdout|stderr|PTY|shell' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
8. `git diff --check`
|
||||
|
||||
Expected: packet 10 is uniquely complete; process and Node mapping tests pass under race; root rename/replacement cannot redirect cwd; Darwin compilation succeeds; exact-template boundaries are documented. Cached tests are not acceptable.
|
||||
|
||||
**After completing all code changes, fill implementation-owned sections in `CODE_REVIEW-*-G??.md`.**
|
||||
|
|
@ -0,0 +1,162 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/11+10_workspace_command plan=0 tag=API milestone-task=tool-executor -->
|
||||
|
||||
# Code Review Reference - API
|
||||
|
||||
> **[IMPLEMENTING AGENT — READ FIRST] Filling in this file is the mandatory final step of implementation.**
|
||||
> The task is NOT complete until every implementation-owned section below is filled in.
|
||||
> Complete the `Implementation Checklist`; the final checklist item is mandatory before saving.
|
||||
> Fill implementation-owned sections, then stop with active files in place and report ready for review.
|
||||
> Execute the plan's selected root cause, scope, files, and dependency decisions as written. Do not choose another owner, narrow/expand the write boundary, or replace a fix with another verification attempt.
|
||||
> If implementation is blocked, record the exact blocker, attempted commands/output, and resume condition only in implementation-owned evidence fields.
|
||||
> Do not ask the user directly, present choices, call user-input tools, create control-plane stop files, or classify the next state.
|
||||
> Finalization (`Code Review Result`, log rename, `complete.log`, archive moves, `Review-Only Checklist`) is review-agent-only, even after compaction/resume.
|
||||
> Follow the ownership table at the bottom of this file for which sections you own.
|
||||
|
||||
## Overview
|
||||
|
||||
date=2026-08-06
|
||||
task=m-iop-owned-single-request-agent-execution/11+10_workspace_command, plan=0, tag=API
|
||||
|
||||
## For the Review Agent
|
||||
|
||||
> **[REVIEW AGENT ONLY]** The finalization steps below are review-agent only. Implementing agents must not execute this section.
|
||||
|
||||
Compare implementation of each item against source files and verify that output in `Verification Results` matches code.
|
||||
Review completion means the following steps are finished:
|
||||
|
||||
1. Append verdict and `review_rework_count` / `evidence_integrity_failure` routing signals.
|
||||
2. Archive `CODE_REVIEW-cloud-G10.md` → `code_review_cloud_G10_0.log` and `PLAN-cloud-G08.md` → `plan_cloud_G08_0.log`.
|
||||
3. If PASS, write `complete.log` and move active task directory to `agent-task/archive/YYYY/MM/m-iop-owned-single-request-agent-execution/11+10_workspace_command/`. If WARN/FAIL, fully write the next filesystem state required by the code-review skill.
|
||||
4. If PASS, preserve the first-line `milestone-task=tool-executor` metadata in `complete.log` and report it for runtime aggregation. Roadmap state evaluation belongs to `sync-milestone-workstate`.
|
||||
5. Check applicable `Review-Only Checklist` items at the final `.log` location before reporting.
|
||||
|
||||
---
|
||||
|
||||
## Implementation Item Completion
|
||||
|
||||
| Item | Status |
|
||||
|------|---------|
|
||||
| API-1 Implement exact-template process execution | [ ] |
|
||||
| API-2 Activate typed command and cancel handling | [ ] |
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Resolve only operator-defined command ids to absolute executable/fixed args and build a minimal allowlisted environment in the fixed workspace cwd.
|
||||
- [ ] Own Unix process groups with one terminal result across exit, timeout, context cancel, explicit cancel, and shared stdout/stderr truncation races.
|
||||
- [ ] Integrate command/cancel into the workspace runtime and Node handler without touching provider cancellation or permitting shell/PTY/arbitrary argv.
|
||||
- [ ] Prove success/nonzero/timeout/cancel/group-child/output/env/cross-request behavior and synchronize command contract/spec limits.
|
||||
- [ ] Run dependency, focused race, package, vet, cross-build, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Review-Only Checklist
|
||||
|
||||
> **[REVIEW AGENT ONLY]** Implementers must not modify/check this section.
|
||||
|
||||
- [ ] Append verdict, routing signals, dimensions, and findings.
|
||||
- [ ] Archive the active pair to routed suffix `0` logs.
|
||||
- [ ] Verify managed `.gitignore` entries.
|
||||
- [ ] On PASS write `complete.log`, preserve/report Milestone metadata, move the directory, and keep the active parent while siblings remain.
|
||||
- [ ] On WARN/FAIL write only the required next state.
|
||||
|
||||
## Deviations from Plan
|
||||
|
||||
_Record deviations and rationale._
|
||||
|
||||
## Key Design Decisions
|
||||
|
||||
_Record implemented decisions._
|
||||
|
||||
## Reviewer Checkpoints
|
||||
|
||||
- Confirm executable and args come only from the approved template; caller supplies no shell/arbitrary argv.
|
||||
- Confirm cwd is fixed, environment is explicit/allowlisted, and no ambient secret is inherited.
|
||||
- Confirm one wait/result owner and entire process-group termination for every cancel/timeout race.
|
||||
- Confirm stdout/stderr share a cap while overflow drains, and cross-request cancel cannot kill another group.
|
||||
|
||||
## Verification Results
|
||||
|
||||
Paste actual stdout/stderr for each command; record replacements under deviations.
|
||||
|
||||
### 1. Dependency
|
||||
|
||||
`test -f agent-task/m-iop-owned-single-request-agent-execution/10+09_workspace_files/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/10+09_workspace_files/complete.log' | wc -l)" -eq 1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 2. Process race tests
|
||||
|
||||
`go test -race ./apps/node/internal/workspace -run 'Test(CommandExecutor|WorkspaceCommandHelperProcess)' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 3. Handler race tests
|
||||
|
||||
`go test -race ./apps/node/internal/node -run 'TestNodeWorkspace(Command|Cancel)' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 4. Package regression
|
||||
|
||||
`go test ./apps/node/internal/workspace ./apps/node/internal/node ./apps/node/internal/transport -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 5. Vet
|
||||
|
||||
`go vet ./apps/node/internal/workspace ./apps/node/internal/node`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 6. Darwin compile
|
||||
|
||||
`GOOS=darwin GOARCH=arm64 go test -c -o /tmp/iop-workspace-command-darwin.test ./apps/node/internal/workspace`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 7. Contract/spec search
|
||||
|
||||
`rg --sort path -n 'command id|fixed args|process group|environment allowlist|stdout|stderr|PTY|shell' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 8. Whitespace
|
||||
|
||||
`git diff --check`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
> **[IMPLEMENTING AGENT — BEFORE SAVING] Have you filled in every implementation-owned section?**
|
||||
> If anything is blank, go back and fill it in before saving this file.
|
||||
> Leave review-agent-only sections unchanged.
|
||||
|
||||
## Section Ownership
|
||||
|
||||
| Section | Owner | Note |
|
||||
|---------|-------|------|
|
||||
| Header comment, Overview, Review Agent Instructions | Fixed at stub creation | Implementing agent must not modify or execute these (archive, complete.log, and task-directory archive move are review-agent only) |
|
||||
| Archive Evidence Snapshot | Fixed at stub creation from plan when present | Implementing agent uses it as default prior-loop context; read only the specific archive files cited there when more detail is required |
|
||||
| Implementation Item Completion (item names) | Fixed at stub creation | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Implementation Checklist (item text/order) | Fixed at stub creation from plan | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Review-Only Checklist | Review agent only | Implementing agent must not modify or check this section |
|
||||
| Deviations from Plan, Key Design Decisions | Implementing agent | Replace placeholder text with actual content |
|
||||
| Reviewer Checkpoints | Fixed at stub creation | Pre-filled from plan |
|
||||
| Verification Results (section headings + commands) | Fixed at stub creation | Implementing agent fills in command output only; command changes require a `Deviations from Plan` entry |
|
||||
| Code Review Result | Review agent appends | Not included in stub |
|
||||
|
|
@ -0,0 +1,206 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/11+10_workspace_command plan=0 tag=API milestone-task=tool-executor -->
|
||||
|
||||
# Bounded Workspace Command Executor
|
||||
|
||||
## For the Implementing Agent
|
||||
|
||||
Do not start until packet 10 has `complete.log`. Use only operator-owned exact command templates, implement within the listed boundary, run every verification command, and fill `CODE_REVIEW-cloud-G10.md`. Do not introduce shell/PTY/general argv execution or own review finalization.
|
||||
|
||||
## Background
|
||||
|
||||
Packet 10 deliberately returns typed unsupported for command. This packet activates only exact operator-configured command ids in the fixed workspace cwd, with environment allowlisting, shared output bounds, deadline, process-group cancellation, and race-safe result ownership.
|
||||
|
||||
## Analysis
|
||||
|
||||
### Files Read
|
||||
|
||||
- `AGENTS.md`
|
||||
- `agent-ops/rules/project/rules.md`
|
||||
- `agent-ops/rules/common/rules-roadmap.md`
|
||||
- `agent-ops/rules/common/rules-agent-spec.md`
|
||||
- `agent-ops/rules/project/domain/node/rules.md`
|
||||
- `agent-ops/rules/project/domain/platform-common/rules.md`
|
||||
- `agent-test/local/rules.md`
|
||||
- `agent-test/local/node-smoke.md`
|
||||
- `agent-roadmap/phase/knowledge-tool-optimization-extension/milestones/iop-owned-single-request-agent-execution.md`
|
||||
- `agent-roadmap/sdd/knowledge-tool-optimization-extension/iop-owned-single-request-agent-execution/SDD.md`
|
||||
- `apps/node/internal/node/node.go`
|
||||
- `apps/node/internal/node/cancel_handler.go`
|
||||
- `apps/node/internal/node/run_handler.go`
|
||||
- `apps/node/internal/transport/session.go`
|
||||
- `packages/go/config/edge_types.go`
|
||||
- `agent-contract/inner/edge-node-runtime-wire.md`
|
||||
- `agent-contract/inner/execution-runtime.md`
|
||||
- `agent-spec/runtime/edge-node-execution.md`
|
||||
|
||||
### SDD Criteria
|
||||
|
||||
- S05 requires command success/failure/timeout/large output plus process cancel and consistent typed results.
|
||||
- D06 excludes interactive terminal, shell, desktop, scheduler, and long-lived agent processes.
|
||||
- Fixed cwd, process group, output cap, timeout, and environment allowlist are mandatory and fail closed.
|
||||
|
||||
### Verification Context
|
||||
|
||||
- Current source has no Node subprocess path; the only `exec.Command` use is host setup, so there is no compatible executor to extend.
|
||||
- Target is Mac, while CI host is Linux. Unix process-group implementation must be build-tagged for Darwin/Linux and tested on Linux; an unsupported fallback keeps other builds explicit.
|
||||
- Tests use the Go test binary as an exact configured executable, not `/bin/sh`.
|
||||
|
||||
### State and Concurrency Findings
|
||||
|
||||
- One tool call owns one process group and one terminal result; timeout, explicit cancel, context cancel, exit, and output overflow race through a single completion path.
|
||||
- Output writers must share one total cap and continue draining after truncation so child pipes cannot deadlock.
|
||||
- Cancel addresses only `(execution_id, tool_call_id)` and cannot kill another request's process.
|
||||
|
||||
### Test Coverage Gaps
|
||||
|
||||
- No exact-template command lookup, minimal environment builder, process group owner, capped writer, or cancel race exists.
|
||||
|
||||
### Symbol References
|
||||
|
||||
- Packet 07 defines command templates; packet 09 defines command/cancel messages; packet 10 owns the runtime and Node handler.
|
||||
- Do not reuse provider `runManager`, `OnCancel`, `exec.CommandContext`'s single-process kill, or any caller shell codec.
|
||||
|
||||
### Split Judgment
|
||||
|
||||
- Command/process correctness is one indivisible slice: start, output drain, timeout/cancel group kill, and wait/result ownership must be reviewed together.
|
||||
- Cleanup of request artifacts and all processes remains packet 13, which builds on this per-tool primitive.
|
||||
|
||||
### Scope Rationale
|
||||
|
||||
- Include exact command id execution, cwd/env/bounds, Unix group lifecycle, typed cancel/result, tests, contract/spec.
|
||||
- Exclude arbitrary argv/shell, PTY, network sandbox claims, cleanup orchestration, provider loop, and public output.
|
||||
|
||||
### Final Routing
|
||||
|
||||
- `evaluation_mode=first-pass`; build closures true, scores 1/2/2/1/2 = G08.
|
||||
- Finalizer route `risk-boundary`, lane `cloud`, filename `PLAN-cloud-G08.md`; risks `temporal_state`, `concurrent_consistency`, `boundary_contract`, `variant_product` (4).
|
||||
- Review scores 2/2/2/2/2 = G10; official filename `CODE_REVIEW-cloud-G10.md`; no recovery/capability gap.
|
||||
|
||||
## Dependencies and Execution Order
|
||||
|
||||
1. Require packet 10 completion.
|
||||
2. Implement capped output and process-group lifecycle before runtime dispatch.
|
||||
3. Wire command and cancel through the existing workspace handler, then update docs.
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Resolve only operator-defined command ids to absolute executable/fixed args and build a minimal allowlisted environment in the fixed workspace cwd.
|
||||
- [ ] Own Unix process groups with one terminal result across exit, timeout, context cancel, explicit cancel, and shared stdout/stderr truncation races.
|
||||
- [ ] Integrate command/cancel into the workspace runtime and Node handler without touching provider cancellation or permitting shell/PTY/arbitrary argv.
|
||||
- [ ] Prove success/nonzero/timeout/cancel/group-child/output/env/cross-request behavior and synchronize command contract/spec limits.
|
||||
- [ ] Run dependency, focused race, package, vet, cross-build, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Implementation Plan
|
||||
|
||||
### [API-1] Implement exact-template process execution
|
||||
|
||||
**Problem**
|
||||
|
||||
- Packet 10's planned `file_executor.go` returns unsupported for command; no process owner or bounded writer exists.
|
||||
- `apps/node/internal/node/cancel_handler.go:12` cancels provider runs and must not be overloaded with workspace process identity.
|
||||
|
||||
**Solution**
|
||||
|
||||
No command executor exists. Add:
|
||||
|
||||
```go
|
||||
type commandExecution struct {
|
||||
done chan struct{}
|
||||
cancelOnce sync.Once
|
||||
// process/result state guarded by one owner
|
||||
}
|
||||
|
||||
func (r *Runtime) executeCommand(ctx context.Context, request Request, input CommandInput) Result
|
||||
func (r *Runtime) Cancel(executionID, toolCallID string) CancelResult
|
||||
```
|
||||
|
||||
Resolve `command_id` to the immutable config template and run its absolute executable plus fixed args only. Reject request argv, unknown ids, disabled capability, unapproved env names/invalid values, and timeout/output bounds. Set `cmd.Dir` to the configured root; set an explicit minimal environment containing only allowed request entries (no implicit inheritance). Route stdout/stderr through one concurrency-safe total byte budget, retain separate bounded streams, mark truncation, and continue discarding overflow.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/node/internal/workspace/command_executor.go` — lookup, minimal env, start/wait/result arbitration, shared capped output.
|
||||
- [ ] `apps/node/internal/workspace/command_process_unix.go` — Darwin/Linux process group creation and group signal/kill.
|
||||
- [ ] `apps/node/internal/workspace/command_process_other.go` — explicit unsupported fallback for non-Unix builds.
|
||||
- [ ] `apps/node/internal/workspace/command_executor_test.go` — helper-process success, exit, cwd/env, timeout, cancel, child group, output, and request isolation.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Use `os.Executable()` plus `-test.run=TestWorkspaceCommandHelperProcess` as the exact template. The helper emits stdout/stderr, spawns a child, blocks, exits non-zero, and records cwd/env as directed.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/node/internal/workspace -run 'Test(CommandExecutor|WorkspaceCommandHelperProcess)' -count=1`
|
||||
- Expected: one typed outcome wins every race, descendants die, and retained output never exceeds the shared cap.
|
||||
|
||||
### [API-2] Activate typed command and cancel handling
|
||||
|
||||
**Problem**
|
||||
|
||||
- Packet 10 leaves `WorkspaceToolOperation_COMMAND` and `WorkspaceCancelRequest` unsupported at the Node handler boundary.
|
||||
|
||||
**Solution**
|
||||
|
||||
Before (packet 10 contract):
|
||||
|
||||
```go
|
||||
case iop.WORKSPACE_TOOL_OPERATION_COMMAND:
|
||||
return unsupportedResult(req)
|
||||
```
|
||||
|
||||
After:
|
||||
|
||||
```go
|
||||
case iop.WORKSPACE_TOOL_OPERATION_COMMAND:
|
||||
return n.workspaceRuntime.Execute(ctx, decodeCommand(req))
|
||||
```
|
||||
|
||||
Decode only command id/env/timeout/output cap; validate identity before dispatch. Implement `OnWorkspaceCancel` through the workspace runtime, make duplicate cancel idempotent, and return typed not-found without touching another execution. Keep cleanup unsupported.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/node/internal/workspace/runtime.go` — track active commands by exact request/tool identity and expose race-safe cancel.
|
||||
- [ ] `apps/node/internal/node/workspace_handler.go` — decode command and map cancel/result status.
|
||||
- [ ] `apps/node/internal/node/workspace_handler_test.go` — command/cancel mapping, duplicate/not-found, and raw-free errors.
|
||||
- [ ] `agent-contract/inner/edge-node-runtime-wire.md` — define exact-template trust boundary, cwd/env/output/process semantics, and exclusions.
|
||||
- [ ] `agent-spec/runtime/edge-node-execution.md` — mark command/cancel implemented with named tests.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Extend direct Node handler tests and assert raw sentinels never appear in logged/typed error text.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/node/internal/node -run 'TestNodeWorkspace(Command|Cancel)' -count=1`
|
||||
- `rg --sort path -n 'command id|fixed args|process group|environment allowlist|stdout|stderr|PTY|shell' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
- Expected: typed command/cancel is active and the fixed-template/non-interactive boundary is explicit.
|
||||
|
||||
## Modified Files Summary
|
||||
|
||||
| File | Item |
|
||||
|------|------|
|
||||
| `apps/node/internal/workspace/command_executor.go` | API-1 |
|
||||
| `apps/node/internal/workspace/command_process_unix.go` | API-1 |
|
||||
| `apps/node/internal/workspace/command_process_other.go` | API-1 |
|
||||
| `apps/node/internal/workspace/command_executor_test.go` | API-1 |
|
||||
| `apps/node/internal/workspace/runtime.go` | API-2 |
|
||||
| `apps/node/internal/node/workspace_handler.go` | API-2 |
|
||||
| `apps/node/internal/node/workspace_handler_test.go` | API-2 |
|
||||
| `agent-contract/inner/edge-node-runtime-wire.md` | API-2 |
|
||||
| `agent-spec/runtime/edge-node-execution.md` | API-2 |
|
||||
| `agent-task/m-iop-owned-single-request-agent-execution/11+10_workspace_command/CODE_REVIEW-cloud-G10.md` | API-1, API-2 |
|
||||
|
||||
## Final Verification
|
||||
|
||||
1. `test -f agent-task/m-iop-owned-single-request-agent-execution/10+09_workspace_files/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/10+09_workspace_files/complete.log' | wc -l)" -eq 1`
|
||||
2. `go test -race ./apps/node/internal/workspace -run 'Test(CommandExecutor|WorkspaceCommandHelperProcess)' -count=1`
|
||||
3. `go test -race ./apps/node/internal/node -run 'TestNodeWorkspace(Command|Cancel)' -count=1`
|
||||
4. `go test ./apps/node/internal/workspace ./apps/node/internal/node ./apps/node/internal/transport -count=1`
|
||||
5. `go vet ./apps/node/internal/workspace ./apps/node/internal/node`
|
||||
6. `GOOS=darwin GOARCH=arm64 go test -c -o /tmp/iop-workspace-command-darwin.test ./apps/node/internal/workspace`
|
||||
7. `rg --sort path -n 'command id|fixed args|process group|environment allowlist|stdout|stderr|PTY|shell' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
8. `git diff --check`
|
||||
|
||||
Expected: packet 10 is uniquely complete; process and Node mapping tests pass under race; Darwin compilation succeeds; exact-template boundaries are documented. Cached tests are not acceptable.
|
||||
|
||||
**After completing all code changes, fill implementation-owned sections in `CODE_REVIEW-*-G??.md`.**
|
||||
|
|
@ -0,0 +1,171 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/12+05,08,11_internal_tool_loop plan=0 tag=API milestone-task=tool-loop -->
|
||||
|
||||
# Code Review Reference - API
|
||||
|
||||
> **[IMPLEMENTING AGENT — READ FIRST] Filling in this file is the mandatory final step of implementation.**
|
||||
> The task is NOT complete until every implementation-owned section below is filled in.
|
||||
> Complete the `Implementation Checklist`; the final checklist item is mandatory before saving.
|
||||
> Fill implementation-owned sections, then stop with active files in place and report ready for review.
|
||||
> Execute the plan's selected root cause, scope, files, and dependency decisions as written. Do not choose another owner, narrow/expand the write boundary, or replace a fix with another verification attempt.
|
||||
> If implementation is blocked, record the exact blocker, attempted commands/output, and resume condition only in implementation-owned evidence fields.
|
||||
> Do not ask the user directly, present choices, call user-input tools, create control-plane stop files, or classify the next state.
|
||||
> Finalization (`Code Review Result`, log rename, `complete.log`, archive moves, `Review-Only Checklist`) is review-agent-only, even after compaction/resume.
|
||||
> Follow the ownership table at the bottom of this file for which sections you own.
|
||||
|
||||
## Overview
|
||||
|
||||
date=2026-08-06
|
||||
task=m-iop-owned-single-request-agent-execution/12+05,08,11_internal_tool_loop, plan=0, tag=API
|
||||
|
||||
## For the Review Agent
|
||||
|
||||
> **[REVIEW AGENT ONLY]** The finalization steps below are review-agent only. Implementing agents must not execute this section.
|
||||
|
||||
Compare implementation of each item against source files and verify that output in `Verification Results` matches code.
|
||||
Review completion means the following steps are finished:
|
||||
|
||||
1. Append verdict and `review_rework_count` / `evidence_integrity_failure` routing signals.
|
||||
2. Archive `CODE_REVIEW-cloud-G10.md` → `code_review_cloud_G10_0.log` and `PLAN-cloud-G09.md` → `plan_cloud_G09_0.log`.
|
||||
3. If PASS, write `complete.log` and move active task directory to `agent-task/archive/YYYY/MM/m-iop-owned-single-request-agent-execution/12+05,08,11_internal_tool_loop/`. If WARN/FAIL, fully write the next filesystem state required by the code-review skill.
|
||||
4. If PASS, preserve the first-line `milestone-task=tool-loop` metadata in `complete.log` and report it for runtime aggregation. Roadmap state evaluation belongs to `sync-milestone-workstate`.
|
||||
5. Check applicable `Review-Only Checklist` items at the final `.log` location before reporting.
|
||||
|
||||
---
|
||||
|
||||
## Implementation Item Completion
|
||||
|
||||
| Item | Status |
|
||||
|------|---------|
|
||||
| API-1 Define canonical internal tool continuation | [ ] |
|
||||
| API-2 Execute and resume the saved stage internally | [ ] |
|
||||
| API-3 Prove no external continuation at the HTTP boundary | [ ] |
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Define closed canonical internal workspace calls/results and strict per-operation decoding independent of caller-facing tool codecs.
|
||||
- [ ] Execute ordered calls through the admitted generation, correlate exactly one pending call/result, resume only the saved stage, and enforce immutable iteration/output/deadline budgets.
|
||||
- [ ] Propagate cancellation and every malformed/stale/denied/exhausted outcome internally with no external continuation or fallback/reselection.
|
||||
- [ ] Prove a real marked Anthropic POST performs multiple Node round trips yet emits no public tool protocol or second ingress, then synchronize contracts/specs.
|
||||
- [ ] Run all dependency, focused race, endpoint, package, vet, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Review-Only Checklist
|
||||
|
||||
> **[REVIEW AGENT ONLY]** Implementers must not modify/check this section.
|
||||
|
||||
- [ ] Append verdict, routing signals, dimensions, and findings.
|
||||
- [ ] Archive the active pair to routed suffix `0` logs and verify `.gitignore`.
|
||||
- [ ] On PASS write `complete.log`, preserve/report Milestone metadata, move this directory, and keep the parent while siblings remain.
|
||||
- [ ] On WARN/FAIL write only the official next loop state.
|
||||
|
||||
## Deviations from Plan
|
||||
|
||||
_Record deviations and rationale._
|
||||
|
||||
## Key Design Decisions
|
||||
|
||||
_Record implemented decisions._
|
||||
|
||||
## Reviewer Checkpoints
|
||||
|
||||
- Confirm service-owned schemas do not call route-01 caller codecs.
|
||||
- Confirm strict decode/capability checks precede wire effects and tool calls execute in order.
|
||||
- Confirm exact request/stage/tool/generation correlation, budget enforcement, and one continuation delivery.
|
||||
- Confirm cancellation sends Node cancel and no tool event reaches surface progress/terminal.
|
||||
- Confirm the real HTTP assertion proves one ingress, multiple tool round trips, no `tool_use`, and one terminal.
|
||||
|
||||
## Verification Results
|
||||
|
||||
Paste actual stdout/stderr for every command and record replacements under deviations.
|
||||
|
||||
### 1. Packet 05 dependency
|
||||
|
||||
`test -f agent-task/m-iop-owned-single-request-agent-execution/05+03_single_ingress/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/05+03_single_ingress/complete.log' | wc -l)" -eq 1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 2. Packet 08 dependency
|
||||
|
||||
`test -f agent-task/m-iop-owned-single-request-agent-execution/08+03,07_workspace_admission/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/08+03,07_workspace_admission/complete.log' | wc -l)" -eq 1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 3. Packet 11 dependency
|
||||
|
||||
`test -f agent-task/m-iop-owned-single-request-agent-execution/11+10_workspace_command/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/11+10_workspace_command/complete.log' | wc -l)" -eq 1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 4. Service race tests
|
||||
|
||||
`go test -race ./apps/edge/internal/service -run 'Test(InternalWorkspaceTool|SingleRequestInternalToolLoop)' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 5. HTTP evidence
|
||||
|
||||
`go test ./apps/edge/internal/openai -run 'TestAnthropicSingleRequest(UsesOnePost|InternalToolsStayPrivate)' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 6. Package regression
|
||||
|
||||
`go test ./apps/edge/internal/service ./apps/edge/internal/openai -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 7. Vet
|
||||
|
||||
`go vet ./apps/edge/internal/service ./apps/edge/internal/openai`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 8. Contract/spec search
|
||||
|
||||
`rg --sort path -n 'internal tool|tool_use|second|workspace|defer' agent-contract/outer/anthropic-compatible-api.md agent-spec/input/openai-compatible-surface.md agent-spec/runtime/edge-node-execution.md`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 9. Whitespace
|
||||
|
||||
`git diff --check`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
> **[IMPLEMENTING AGENT — BEFORE SAVING] Have you filled in every implementation-owned section?**
|
||||
> If anything is blank, go back and fill it in before saving this file.
|
||||
> Leave review-agent-only sections unchanged.
|
||||
|
||||
## Section Ownership
|
||||
|
||||
| Section | Owner | Note |
|
||||
|---------|-------|------|
|
||||
| Header comment, Overview, Review Agent Instructions | Fixed at stub creation | Implementing agent must not modify or execute these (archive, complete.log, and task-directory archive move are review-agent only) |
|
||||
| Archive Evidence Snapshot | Fixed at stub creation from plan when present | Implementing agent uses it as default prior-loop context; read only the specific archive files cited there when more detail is required |
|
||||
| Implementation Item Completion (item names) | Fixed at stub creation | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Implementation Checklist (item text/order) | Fixed at stub creation from plan | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Review-Only Checklist | Review agent only | Implementing agent must not modify or check this section |
|
||||
| Deviations from Plan, Key Design Decisions | Implementing agent | Replace placeholder text with actual content |
|
||||
| Reviewer Checkpoints | Fixed at stub creation | Pre-filled from plan |
|
||||
| Verification Results (section headings + commands) | Fixed at stub creation | Implementing agent fills in command output only; command changes require a `Deviations from Plan` entry |
|
||||
| Code Review Result | Review agent appends | Not included in stub |
|
||||
|
|
@ -0,0 +1,237 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/12+05,08,11_internal_tool_loop plan=0 tag=API milestone-task=tool-loop -->
|
||||
|
||||
# Coordinator-owned Internal Workspace Tool Loop
|
||||
|
||||
## For the Implementing Agent
|
||||
|
||||
Do not start until packets 05, 08, and 11 each have `complete.log`. Implement the coordinator/tool continuation exactly within the listed boundary, run all verification, fill `CODE_REVIEW-cloud-G10.md`, and leave finalization to official review. Do not reuse caller continuation or activate an unplanned production stage driver.
|
||||
|
||||
## Background
|
||||
|
||||
The coordinator recognizes an `internal_tool` detour and the Node can execute tools, but no owner validates model tool calls, sends them over the admitted wire, and returns results to the same internal execution without exposing `tool_use`. This packet closes that loop as an injectable service capability; plan/work/review provider drivers remain their own Epic.
|
||||
|
||||
## Analysis
|
||||
|
||||
### Files Read
|
||||
|
||||
- `AGENTS.md`
|
||||
- `agent-ops/rules/project/rules.md`
|
||||
- `agent-ops/rules/common/rules-roadmap.md`
|
||||
- `agent-ops/rules/common/rules-agent-spec.md`
|
||||
- `agent-ops/rules/project/domain/edge/rules.md`
|
||||
- `agent-test/local/rules.md`
|
||||
- `agent-test/local/edge-smoke.md`
|
||||
- `agent-roadmap/phase/knowledge-tool-optimization-extension/milestones/iop-owned-single-request-agent-execution.md`
|
||||
- `agent-roadmap/sdd/knowledge-tool-optimization-extension/iop-owned-single-request-agent-execution/SDD.md`
|
||||
- `agent-task/m-iop-owned-single-request-agent-execution/03+02_single_request_coordinator/PLAN-local-G07.md`
|
||||
- `agent-task/m-iop-owned-single-request-agent-execution/05+03_single_ingress/PLAN-cloud-G09.md`
|
||||
- `apps/edge/internal/service/service.go`
|
||||
- `apps/edge/internal/openai/server.go`
|
||||
- `apps/edge/internal/openai/anthropic_handler.go`
|
||||
- `apps/edge/internal/openai/anthropic_types.go`
|
||||
- `apps/edge/internal/openai/workspace_tool_binding.go`
|
||||
- `apps/edge/internal/openai/workspace_tool_codec.go`
|
||||
- `agent-contract/outer/anthropic-compatible-api.md`
|
||||
- `agent-contract/inner/edge-node-runtime-wire.md`
|
||||
- `agent-spec/input/openai-compatible-surface.md`
|
||||
- `agent-spec/runtime/edge-node-execution.md`
|
||||
|
||||
### SDD Criteria
|
||||
|
||||
- S06 requires multiple internal model tool call/result round trips, zero Claude-facing `tool_use` terminal, and no second HTTP request.
|
||||
- `internal_tool` resumes only the saved active stage. Stage/request iteration, output, and deadline limits are immutable and exhaustion fails closed.
|
||||
- D04/D10 keep provider reasoning/tool protocol private; public progress/final output is owned by packets 05/06.
|
||||
|
||||
### Verification Context
|
||||
|
||||
- Packet 03 defines coordinator envelopes and terminal ownership; packet 05 proves one real marked POST; packet 08 supplies immutable workspace admission; packet 11 supplies all canonical Node operations.
|
||||
- These APIs do not exist at starting HEAD, so dependency completion and exact post-implementation interfaces are mandatory preflight.
|
||||
- Service race tests plus packet 05's real HTTP test are the deterministic oracle; no real provider or Mac runner is needed.
|
||||
|
||||
### State and Concurrency Findings
|
||||
|
||||
- Tool calls are ordered workspace effects; execute sequentially unless a later SDD explicitly adds parallel semantics.
|
||||
- A tool result must correlate request, stage, tool id, workspace ref, Node generation, and exactly one pending call.
|
||||
- Duplicate/malformed/unknown/capability-denied calls, repeated ids, stale result, budget exhaustion, and cancel fail the request without an external continuation.
|
||||
|
||||
### Test Coverage Gaps
|
||||
|
||||
- The current caller-owned workspace codecs translate public tool schemas and must not be reused as ownership.
|
||||
- No service capability consumes a canonical internal call and supplies its result back to the same executor handle.
|
||||
|
||||
### Symbol References
|
||||
|
||||
- Extend packet 03's executor/handle via a separate optional continuation interface so existing fakes remain compatible.
|
||||
- Packet 05's separate `singleRequestService` branch remains the only HTTP branch; do not widen `runService`.
|
||||
- Existing route-01 `workspace_tool_*` code stays unchanged and caller-owned.
|
||||
|
||||
### Split Judgment
|
||||
|
||||
- The decode/correlate/wire/result/resume invariant is atomic and independently PASS-capable with fake executor plus net-pipe Node.
|
||||
- Provider-specific plan/work/review prompts and repair policy are excluded and consume this port later.
|
||||
|
||||
### Scope Rationale
|
||||
|
||||
- Include canonical internal schemas, strict decoding, ordered loop, workspace open, continuation correlation, budget/cancel, service and HTTP evidence, contract/spec.
|
||||
- Exclude provider drivers, stage prompts, cleanup effects, streaming progress, observation metrics, and real Claude smoke.
|
||||
|
||||
### Final Routing
|
||||
|
||||
- `evaluation_mode=first-pass`; build closures true, scores 2/2/2/1/2 = G09.
|
||||
- Finalizer route `grade-boundary`, lane `cloud`, filename `PLAN-cloud-G09.md`; all five loop-risk signatures are positive.
|
||||
- Review scores 2/2/2/2/2 = G10; official filename `CODE_REVIEW-cloud-G10.md`; no capability/recovery gap.
|
||||
|
||||
## Dependencies and Execution Order
|
||||
|
||||
1. Require packet 05 for the marked HTTP branch/evidence.
|
||||
2. Require packet 08 for immutable workspace identity/capabilities.
|
||||
3. Require packet 11 for complete file/command/cancel execution.
|
||||
4. Define schemas and continuation interface, implement the loop, then extend real-POST evidence/docs.
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Define closed canonical internal workspace calls/results and strict per-operation decoding independent of caller-facing tool codecs.
|
||||
- [ ] Execute ordered calls through the admitted generation, correlate exactly one pending call/result, resume only the saved stage, and enforce immutable iteration/output/deadline budgets.
|
||||
- [ ] Propagate cancellation and every malformed/stale/denied/exhausted outcome internally with no external continuation or fallback/reselection.
|
||||
- [ ] Prove a real marked Anthropic POST performs multiple Node round trips yet emits no public tool protocol or second ingress, then synchronize contracts/specs.
|
||||
- [ ] Run all dependency, focused race, endpoint, package, vet, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Implementation Plan
|
||||
|
||||
### [API-1] Define canonical internal tool continuation
|
||||
|
||||
**Problem**
|
||||
|
||||
- Packet 03 plans `internal_tool` as a state detour but its executor port has no concrete Node tool result continuation.
|
||||
- `apps/edge/internal/openai/workspace_tool_codec.go:1` belongs to caller-facing route-01 behavior and cannot own S06.
|
||||
|
||||
**Solution**
|
||||
|
||||
Add service-owned types, with no OpenAI/Anthropic import:
|
||||
|
||||
```go
|
||||
type InternalWorkspaceToolCall struct {
|
||||
RequestID, StageID, ToolCallID, Name string
|
||||
Arguments json.RawMessage
|
||||
}
|
||||
|
||||
type InternalWorkspaceToolResult struct {
|
||||
RequestID, StageID, ToolCallID string
|
||||
Status, ErrorCode string
|
||||
// bounded typed result fields
|
||||
}
|
||||
|
||||
type SingleRequestToolContinuation interface {
|
||||
ContinueInternalTool(context.Context, InternalWorkspaceToolResult) error
|
||||
}
|
||||
```
|
||||
|
||||
Use exactly `workspace_read`, `workspace_list`, `workspace_write`, `workspace_delete`, and `workspace_command`. Decode each with `json.Decoder.DisallowUnknownFields`, reject trailing data/unknown fields/empty identity/invalid combinations, and map to packet 09 typed inputs. Command accepts only command id and allowlisted env values, never executable/argv.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/edge/internal/service/single_request_tool_types.go` — canonical calls/results, strict decoders, clone/redaction helpers.
|
||||
- [ ] `apps/edge/internal/service/single_request_tool_types_test.go` — valid operation table and malformed/unknown/trailing/identity/capability cases.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Table-test each schema, malicious path/command shapes, duplicate ids, raw sentinel redaction, and deep-copy behavior.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test ./apps/edge/internal/service -run 'TestInternalWorkspaceTool(Call|Decode)' -count=1`
|
||||
- Expected: only the closed canonical schema reaches wire DTOs and failures expose no raw arguments.
|
||||
|
||||
### [API-2] Execute and resume the saved stage internally
|
||||
|
||||
**Problem**
|
||||
|
||||
- Packet 03's planned `StartSingleRequest` validates state but does not own workspace open/tool/result delivery.
|
||||
- Packet 09's wire methods are not connected to coordinator envelopes.
|
||||
|
||||
**Solution**
|
||||
|
||||
Before (predecessor state contract, `03+02_single_request_coordinator/PLAN-local-G07.md:160`):
|
||||
|
||||
```go
|
||||
// Typed internal envelopes carry request/stage identity.
|
||||
// internal_tool returns only to its saved active stage.
|
||||
```
|
||||
|
||||
After, add a request-local tool loop that opens the frozen workspace once on first call, validates identity/capability, sends one ordered tool at a time, converts the bounded typed response, and calls only the emitting execution's `ContinueInternalTool`. Track pending tool id and iteration/output/deadline budget under the coordinator lock/state discipline. Duplicate/stale continuation is rejected; context cancellation sends typed cancel. Tool envelopes never enter the surface progress/terminal channel.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/edge/internal/service/service.go` — configure/snapshot the optional internal tool executor without endpoint coupling.
|
||||
- [ ] `apps/edge/internal/service/single_request.go` — intercept `internal_tool`, preserve saved stage, and resume through the optional continuation interface.
|
||||
- [ ] `apps/edge/internal/service/single_request_tool_loop.go` — own open/call/result correlation, budgets, cancellation, and generation-fenced wire use.
|
||||
- [ ] `apps/edge/internal/service/single_request_tool_loop_test.go` — fake executor plus net-pipe Node for multi-tool, identity, denial, malformed, stale, budget, cancel, and one terminal races.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Drive read→write→command calls from one fake execution, assert ordered Node requests/results, saved-stage resume, and only one sanitized service terminal.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/edge/internal/service -run 'TestSingleRequestInternalToolLoop' -count=1`
|
||||
- Expected: multiple correlated tools complete internally; all invalid/racing cases fail closed without exposed tool events.
|
||||
|
||||
### [API-3] Prove no external continuation at the HTTP boundary
|
||||
|
||||
**Problem**
|
||||
|
||||
- Packet 05's planned real-POST test proves one ingress with a fake multi-stage executor, but not an actual internal Node tool round trip.
|
||||
|
||||
**Solution**
|
||||
|
||||
Extend the completed packet 05 fixture with packet 12's service tool loop and a net-pipe workspace handler. Send one real marked POST, make the fake executor emit at least two internal tool calls and accept results, then assert ingress counter delta `+1`, one terminal, no `tool_use`/`tool_result`/private sentinel, and no additional HTTP request. Do not change handler production code unless required by a verified integration defect; any such need is outside this write boundary and must be recorded as a blocker for official review.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/edge/internal/openai/single_request_handler_test.go` — add real-POST multi-tool/zero-public-continuation integration evidence.
|
||||
- [ ] `agent-contract/outer/anthropic-compatible-api.md` — mark internal tool continuation implemented and private.
|
||||
- [ ] `agent-spec/input/openai-compatible-surface.md` — link the real-POST multi-tool evidence.
|
||||
- [ ] `agent-spec/runtime/edge-node-execution.md` — document the service loop, identity/budget/cancel behavior, and provider-driver deferral.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Add `TestAnthropicSingleRequestInternalToolsStayPrivate`; docs use this and API-2 tests as executable oracle.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test ./apps/edge/internal/openai -run 'TestAnthropicSingleRequest(UsesOnePost|InternalToolsStayPrivate)' -count=1`
|
||||
- `rg --sort path -n 'internal tool|tool_use|second|workspace|defer' agent-contract/outer/anthropic-compatible-api.md agent-spec/input/openai-compatible-surface.md agent-spec/runtime/edge-node-execution.md`
|
||||
- Expected: one real POST performs multiple internal wire calls and public output contains no continuation protocol.
|
||||
|
||||
## Modified Files Summary
|
||||
|
||||
| File | Item |
|
||||
|------|------|
|
||||
| `apps/edge/internal/service/single_request_tool_types.go` | API-1 |
|
||||
| `apps/edge/internal/service/single_request_tool_types_test.go` | API-1 |
|
||||
| `apps/edge/internal/service/service.go` | API-2 |
|
||||
| `apps/edge/internal/service/single_request.go` | API-2 |
|
||||
| `apps/edge/internal/service/single_request_tool_loop.go` | API-2 |
|
||||
| `apps/edge/internal/service/single_request_tool_loop_test.go` | API-2 |
|
||||
| `apps/edge/internal/openai/single_request_handler_test.go` | API-3 |
|
||||
| `agent-contract/outer/anthropic-compatible-api.md` | API-3 |
|
||||
| `agent-spec/input/openai-compatible-surface.md` | API-3 |
|
||||
| `agent-spec/runtime/edge-node-execution.md` | API-3 |
|
||||
| `agent-task/m-iop-owned-single-request-agent-execution/12+05,08,11_internal_tool_loop/CODE_REVIEW-cloud-G10.md` | API-1, API-2, API-3 |
|
||||
|
||||
## Final Verification
|
||||
|
||||
1. `test -f agent-task/m-iop-owned-single-request-agent-execution/05+03_single_ingress/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/05+03_single_ingress/complete.log' | wc -l)" -eq 1`
|
||||
2. `test -f agent-task/m-iop-owned-single-request-agent-execution/08+03,07_workspace_admission/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/08+03,07_workspace_admission/complete.log' | wc -l)" -eq 1`
|
||||
3. `test -f agent-task/m-iop-owned-single-request-agent-execution/11+10_workspace_command/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/11+10_workspace_command/complete.log' | wc -l)" -eq 1`
|
||||
4. `go test -race ./apps/edge/internal/service -run 'Test(InternalWorkspaceTool|SingleRequestInternalToolLoop)' -count=1`
|
||||
5. `go test ./apps/edge/internal/openai -run 'TestAnthropicSingleRequest(UsesOnePost|InternalToolsStayPrivate)' -count=1`
|
||||
6. `go test ./apps/edge/internal/service ./apps/edge/internal/openai -count=1`
|
||||
7. `go vet ./apps/edge/internal/service ./apps/edge/internal/openai`
|
||||
8. `rg --sort path -n 'internal tool|tool_use|second|workspace|defer' agent-contract/outer/anthropic-compatible-api.md agent-spec/input/openai-compatible-surface.md agent-spec/runtime/edge-node-execution.md`
|
||||
9. `git diff --check`
|
||||
|
||||
Expected: all three predecessors are uniquely complete; multi-tool flow stays internal and ordered under race; one real POST yields one private-free terminal; package checks pass. Cached tests are not acceptable.
|
||||
|
||||
**After completing all code changes, fill implementation-owned sections in `CODE_REVIEW-*-G??.md`.**
|
||||
|
|
@ -0,0 +1,168 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/13+12_workspace_cleanup plan=1 tag=API milestone-task=cleanup-observation -->
|
||||
|
||||
# Code Review Reference - API
|
||||
|
||||
> **[IMPLEMENTING AGENT — READ FIRST] Filling in this file is the mandatory final step of implementation.**
|
||||
> The task is NOT complete until every implementation-owned section below is filled in.
|
||||
> Complete the `Implementation Checklist`; the final checklist item is mandatory before saving.
|
||||
> Fill implementation-owned sections, then stop with active files in place and report ready for review.
|
||||
> Execute the plan's selected root cause, scope, files, and dependency decisions as written. Do not choose another owner, narrow/expand the write boundary, or replace a fix with another verification attempt.
|
||||
> If implementation is blocked, record the exact blocker, attempted commands/output, and resume condition only in implementation-owned evidence fields.
|
||||
> Do not ask the user directly, present choices, call user-input tools, create control-plane stop files, or classify the next state.
|
||||
> Finalization (`Code Review Result`, log rename, `complete.log`, archive moves, `Review-Only Checklist`) is review-agent-only, even after compaction/resume.
|
||||
> Follow the ownership table at the bottom of this file for which sections you own.
|
||||
|
||||
## Overview
|
||||
|
||||
date=2026-08-06
|
||||
task=m-iop-owned-single-request-agent-execution/13+12_workspace_cleanup, plan=1, tag=API
|
||||
|
||||
## Archive Evidence Snapshot
|
||||
|
||||
- The first-pass pair is preserved at `agent-task/m-iop-owned-single-request-agent-execution/13+12_workspace_cleanup/plan_cloud_G09_0.log` and `agent-task/m-iop-owned-single-request-agent-execution/13+12_workspace_cleanup/code_review_cloud_G10_0.log`; it contains no implementation evidence or review verdict.
|
||||
- Self-review found that blind `os.Root.RemoveAll` can cross a mounted subtree and cannot distinguish Node-owned artifacts from injected/unowned entries. Plan 1 uses the immutable `request_id`, an in-memory ownership inventory, no-follow descriptor traversal, and deepest-first non-recursive removal that fails closed on any ownership or filesystem-boundary mismatch.
|
||||
|
||||
## For the Review Agent
|
||||
|
||||
> **[REVIEW AGENT ONLY]** The finalization steps below are review-agent only. Implementing agents must not execute this section.
|
||||
|
||||
Compare implementation of each item against source files and verify that output in `Verification Results` matches code.
|
||||
Review completion means the following steps are finished:
|
||||
|
||||
1. Append verdict and `review_rework_count` / `evidence_integrity_failure` routing signals.
|
||||
2. Archive `CODE_REVIEW-cloud-G10.md` → `code_review_cloud_G10_1.log` and `PLAN-cloud-G09.md` → `plan_cloud_G09_1.log`.
|
||||
3. If PASS, write `complete.log` and move active task directory to `agent-task/archive/YYYY/MM/m-iop-owned-single-request-agent-execution/13+12_workspace_cleanup/`. If WARN/FAIL, fully write the next filesystem state required by the code-review skill.
|
||||
4. If PASS, preserve the first-line `milestone-task=cleanup-observation` metadata in `complete.log` and report it for runtime aggregation. Roadmap state evaluation belongs to `sync-milestone-workstate`.
|
||||
5. Check applicable `Review-Only Checklist` items at the final `.log` location before reporting.
|
||||
|
||||
---
|
||||
|
||||
## Implementation Item Completion
|
||||
|
||||
| Item | Status |
|
||||
|------|---------|
|
||||
| API-1 Reclaim only Node request-owned state | [ ] |
|
||||
| API-2 Complete typed cleanup handling and coordinator finalization | [ ] |
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Create and validate only `.iop/job/<request_id>` from the immutable coordinator identity, inventory every Node-owned artifact, and preserve every user or unowned result.
|
||||
- [ ] Cancel/wait all process groups and remove only inventoried artifacts plus empty owned directories exactly once per request with bounded concurrent/idempotent result ownership.
|
||||
- [ ] Make coordinator success/error/cancel/disconnect paths converge on one typed cleanup before terminal commit, with fail-closed success handling.
|
||||
- [ ] Prove cleanup races, symlink/mount/unowned-entry refusal, user-result preservation, cross-request isolation, failure handling, and synchronize cleanup contract/spec claims.
|
||||
- [ ] Run dependency, focused race, package, vet, Darwin compile, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Review-Only Checklist
|
||||
|
||||
> **[REVIEW AGENT ONLY]** Implementers must not modify/check this section.
|
||||
|
||||
- [ ] Append verdict, routing signals, dimensions, and findings.
|
||||
- [ ] Archive the active pair to routed suffix `1` logs and verify `.gitignore`.
|
||||
- [ ] On PASS write `complete.log`, preserve/report Milestone metadata, move this directory, and keep the active parent while siblings remain.
|
||||
- [ ] On WARN/FAIL write only the required next loop state.
|
||||
|
||||
## Deviations from Plan
|
||||
|
||||
_Record deviations and rationale._
|
||||
|
||||
## Key Design Decisions
|
||||
|
||||
_Record implemented decisions._
|
||||
|
||||
## Reviewer Checkpoints
|
||||
|
||||
- Confirm no recursive removal is used: the exact `.iop/job/<request_id>` tree is no-follow enumerated against the ownership inventory and removed deepest-first with non-recursive descriptor operations.
|
||||
- Confirm symlink, mount/device change, inode replacement, special file, and unowned entry fail closed without deleting suspect/user/sibling content.
|
||||
- Confirm every process group for one request is cancelled/waited and foreign request processes are untouched.
|
||||
- Confirm duplicate/racing cleanup shares one result without unbounded state growth.
|
||||
- Confirm final success waits for cleanup and cleanup failure cannot yield partial success.
|
||||
- Confirm user result files survive success, error, cancel, and runtime close.
|
||||
|
||||
## Verification Results
|
||||
|
||||
Paste actual stdout/stderr for every command; record replacements under deviations.
|
||||
|
||||
### 1. Dependency
|
||||
|
||||
`test -f agent-task/m-iop-owned-single-request-agent-execution/12+05,08,11_internal_tool_loop/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/12+05,08,11_internal_tool_loop/complete.log' | wc -l)" -eq 1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 2. Node cleanup race tests
|
||||
|
||||
`go test -race ./apps/node/internal/workspace -run 'TestWorkspaceCleanup' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 3. Handler/coordinator race tests
|
||||
|
||||
`go test -race ./apps/node/internal/node ./apps/edge/internal/service -run 'Test(NodeWorkspaceCleanup|SingleRequestCleanup)' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 4. Package regression
|
||||
|
||||
`go test ./apps/node/internal/workspace ./apps/node/internal/node ./apps/edge/internal/service -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 5. Vet
|
||||
|
||||
`go vet ./apps/node/internal/workspace ./apps/node/internal/node ./apps/edge/internal/service`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 6. Darwin compile
|
||||
|
||||
`GOOS=darwin GOARCH=arm64 go test -c -o /tmp/iop-workspace-cleanup-darwin.test ./apps/node/internal/workspace`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 7. Contract/spec search
|
||||
|
||||
`rg --sort path -n 'cleanup|request_id|\.iop/job|inventory|no-follow|user result|finalizing|exactly' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 8. Whitespace
|
||||
|
||||
`git diff --check`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
> **[IMPLEMENTING AGENT — BEFORE SAVING] Have you filled in every implementation-owned section?**
|
||||
> If anything is blank, go back and fill it in before saving this file.
|
||||
> Leave review-agent-only sections unchanged.
|
||||
|
||||
## Section Ownership
|
||||
|
||||
| Section | Owner | Note |
|
||||
|---------|-------|------|
|
||||
| Header comment, Overview, Review Agent Instructions | Fixed at stub creation | Implementing agent must not modify or execute these (archive, complete.log, and task-directory archive move are review-agent only) |
|
||||
| Archive Evidence Snapshot | Fixed at stub creation from plan when present | Implementing agent uses it as default prior-loop context; read only the specific archive files cited there when more detail is required |
|
||||
| Implementation Item Completion (item names) | Fixed at stub creation | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Implementation Checklist (item text/order) | Fixed at stub creation from plan | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Review-Only Checklist | Review agent only | Implementing agent must not modify or check this section |
|
||||
| Deviations from Plan, Key Design Decisions | Implementing agent | Replace placeholder text with actual content |
|
||||
| Reviewer Checkpoints | Fixed at stub creation | Pre-filled from plan |
|
||||
| Verification Results (section headings + commands) | Fixed at stub creation | Implementing agent fills in command output only; command changes require a `Deviations from Plan` entry |
|
||||
| Code Review Result | Review agent appends | Not included in stub |
|
||||
|
|
@ -0,0 +1,207 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/13+12_workspace_cleanup plan=1 tag=API milestone-task=cleanup-observation -->
|
||||
|
||||
# Request-owned Workspace Cleanup
|
||||
|
||||
## For the Implementing Agent
|
||||
|
||||
Do not start until packet 12 has `complete.log`. Implement only request-owned process/artifact cleanup in the listed files, preserve user results, run every verification command, and fill `CODE_REVIEW-cloud-G10.md`. Do not broaden cleanup into rollback or own official review state.
|
||||
|
||||
## Background
|
||||
|
||||
The internal loop can open a Node workspace and run processes, but success/error/cancel paths do not yet converge on one cleanup owner. This packet adds exactly-once coordinator cleanup and Node reclamation limited to active command groups and registered Node-owned artifacts under `.iop/job/<request_id>`.
|
||||
|
||||
## Archive Evidence Snapshot
|
||||
|
||||
- The first-pass pair is preserved at `agent-task/m-iop-owned-single-request-agent-execution/13+12_workspace_cleanup/plan_cloud_G09_0.log` and `agent-task/m-iop-owned-single-request-agent-execution/13+12_workspace_cleanup/code_review_cloud_G10_0.log`; it contains no implementation evidence or review verdict.
|
||||
- Self-review found that blind `os.Root.RemoveAll` can cross a mounted subtree and cannot distinguish Node-owned artifacts from injected/unowned entries. Plan 1 uses the immutable `request_id`, an in-memory ownership inventory, no-follow descriptor traversal, and deepest-first non-recursive removal that fails closed on any ownership or filesystem-boundary mismatch.
|
||||
|
||||
## Analysis
|
||||
|
||||
### Files Read
|
||||
|
||||
- `AGENTS.md`
|
||||
- `agent-ops/rules/project/rules.md`
|
||||
- `agent-ops/rules/common/rules-roadmap.md`
|
||||
- `agent-ops/rules/common/rules-agent-spec.md`
|
||||
- `agent-ops/rules/project/domain/edge/rules.md`
|
||||
- `agent-ops/rules/project/domain/node/rules.md`
|
||||
- `agent-test/local/rules.md`
|
||||
- `agent-test/local/edge-smoke.md`
|
||||
- `agent-test/local/node-smoke.md`
|
||||
- `agent-roadmap/phase/knowledge-tool-optimization-extension/milestones/iop-owned-single-request-agent-execution.md`
|
||||
- `agent-roadmap/sdd/knowledge-tool-optimization-extension/iop-owned-single-request-agent-execution/SDD.md`
|
||||
- `agent-task/m-iop-owned-single-request-agent-execution/03+02_single_request_coordinator/PLAN-local-G07.md`
|
||||
- `apps/edge/internal/service/service.go`
|
||||
- `apps/node/internal/node/cancel_handler.go`
|
||||
- `apps/node/internal/node/node.go`
|
||||
- `agent-contract/inner/edge-node-runtime-wire.md`
|
||||
- `agent-spec/runtime/edge-node-execution.md`
|
||||
|
||||
### SDD Criteria
|
||||
|
||||
- S07 requires success, error, and cancel to remove request-owned processes and registered artifacts beneath the exact `.iop/job/<request_id>` namespace while preserving requested workspace results.
|
||||
- `finalizing` waits for cleanup before committing the final terminal. Cleanup never rolls back user files.
|
||||
- S11 caller disconnect must cancel provider/tool processes and perform bounded cleanup without another external request.
|
||||
|
||||
### Verification Context
|
||||
|
||||
- Packet 09 already defines typed cleanup; packets 10/11 own request contexts and process groups; packet 12 owns the coordinator loop.
|
||||
- Race tests use temporary roots and helper processes. No external Mac runner is required for logic, while Darwin compile remains a final gate.
|
||||
- Existing route-01 cleanup is caller-artifact state and is not reusable as Node filesystem ownership.
|
||||
|
||||
### State and Concurrency Findings
|
||||
|
||||
- Success, executor error, request timeout, caller cancel, terminal write failure, and duplicate callbacks may all race to cleanup; only one wire cleanup is sent and all callers observe its result.
|
||||
- Node cleanup cancels every active process for exactly one request, waits boundedly, removes only registered Node-owned internal artifacts without recursive traversal, closes the request context, and caches a bounded idempotent result.
|
||||
- Cleanup failure converts a pending success to failure; existing failure/cancel remains primary while safe cleanup failure is internal evidence.
|
||||
|
||||
### Test Coverage Gaps
|
||||
|
||||
- No Node operation removes request job artifacts or all request process groups.
|
||||
- Coordinator finalization does not wait for workspace cleanup or prove exactly-once across terminal races.
|
||||
|
||||
### Symbol References
|
||||
|
||||
- Add an optional lifecycle interface to packet 12's internal tool executor; do not break fakes that never opened a workspace.
|
||||
- Provider `CancelRun` and route-01 cleanup remain separate.
|
||||
|
||||
### Split Judgment
|
||||
|
||||
- Node cleanup and coordinator exactly-once finalization form one end-to-end ownership invariant and must be reviewed together.
|
||||
- Observation is packet 14 because it can instrument the stable lifecycle without changing cleanup outcomes.
|
||||
|
||||
### Scope Rationale
|
||||
|
||||
- Include job namespace, process cancellation/wait, idempotence, coordinator deferral, user-file preservation, tests, wire contract/spec.
|
||||
- Exclude general rollback, git reset, arbitrary directory cleanup, metrics/logging implementation, and real provider smoke.
|
||||
|
||||
### Final Routing
|
||||
|
||||
- `evaluation_mode=isolated-reassessment`; `review_rework_count=0`; `evidence_integrity_failure=false`; build closures true, scores 2/2/2/1/2 = G09.
|
||||
- Finalizer route `grade-boundary`, lane `cloud`, filename `PLAN-cloud-G09.md`; risks `temporal_state`, `concurrent_consistency`, `boundary_contract`, `variant_product` (4).
|
||||
- Review scores 2/2/2/2/2 = G10; official filename `CODE_REVIEW-cloud-G10.md`; no capability/recovery gap.
|
||||
|
||||
## Dependencies and Execution Order
|
||||
|
||||
1. Require packet 12 completion.
|
||||
2. Implement Node cleanup and race tests.
|
||||
3. Add optional Edge lifecycle cleanup and terminal ordering.
|
||||
4. Synchronize contract/spec only after end-to-end verification.
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Create and validate only `.iop/job/<request_id>` from the immutable coordinator identity, inventory every Node-owned artifact, and preserve every user or unowned result.
|
||||
- [ ] Cancel/wait all process groups and remove only inventoried artifacts plus empty owned directories exactly once per request with bounded concurrent/idempotent result ownership.
|
||||
- [ ] Make coordinator success/error/cancel/disconnect paths converge on one typed cleanup before terminal commit, with fail-closed success handling.
|
||||
- [ ] Prove cleanup races, symlink/mount/unowned-entry refusal, user-result preservation, cross-request isolation, failure handling, and synchronize cleanup contract/spec claims.
|
||||
- [ ] Run dependency, focused race, package, vet, Darwin compile, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Implementation Plan
|
||||
|
||||
### [API-1] Reclaim only Node request-owned state
|
||||
|
||||
**Problem**
|
||||
|
||||
- Packet 11 tracks commands per immutable request identity but has no all-process cleanup or artifact namespace reclamation.
|
||||
- `apps/node/internal/node/cancel_handler.go:11` owns provider run cancel only.
|
||||
|
||||
**Solution**
|
||||
|
||||
Add `cleanup.go` to workspace runtime. On request open, derive `.iop/job/<validated-request-id>` only from packet 10's immutable coordinator identity. Create each internal directory component through a no-follow descriptor helper, verify it remains on the admitted filesystem, and register the exact directory identity in the request's in-memory ownership inventory. Every later Node-owned internal artifact must be created through the same helper and registered by relative path, type, and stable file identity; caller file tools still cannot access `.iop`.
|
||||
|
||||
Cleanup atomically elects one owner, cancels all process groups for that request, and waits up to the admitted cleanup bound. It then compares a no-follow enumeration of the exact job tree with the inventory, rejects symlinks, special files, mount/device changes, identity replacements, and unregistered entries, and removes registered files followed by deepest-first empty directories with descriptor-relative non-recursive unlink/rmdir. Never call `Root.RemoveAll`, never follow an entry during validation/removal, and never widen the path after an error. A mismatch returns typed cleanup failure and preserves the suspect subtree for operator inspection. Concurrent/duplicate callers wait for and receive the same typed result. Retain completed results in a bounded cache; eviction may repeat an idempotent missing-directory check but never broadens scope. Runtime close invokes the same primitive for active requests.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/node/internal/workspace/runtime.go` — request lifecycle state, immutable request artifact inventory, and bounded completed-cleanup cache.
|
||||
- [ ] `apps/node/internal/workspace/cleanup.go` — process cancel/wait, inventory validation, deepest-first non-recursive removal, and idempotent result ownership.
|
||||
- [ ] `apps/node/internal/workspace/cleanup_path_unix.go` — Darwin/Linux descriptor-relative no-follow mkdir/enumerate/identity/unlink/rmdir primitives.
|
||||
- [ ] `apps/node/internal/workspace/cleanup_path_other.go` — explicit unsupported fallback outside Darwin/Linux.
|
||||
- [ ] `apps/node/internal/workspace/cleanup_test.go` — success/error/cancel races, duplicate calls, active child, timeout, symlink/mount/identity replacement/unowned entry, user-result/cross-request preservation, and invalid id.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Create user results beside `.iop`, two requests, nested artifacts only through the internal ownership helper, and helper-process descendants; race cleanup/cancel/close and assert only the inventoried target paths disappear. Inject a sibling job, unregistered file, symlink, replaced inode, special file, and mount/substitute where supported; assert cleanup fails closed without deleting the suspect, user, or sibling content. Skip only the privileged mount fixture when unavailable while retaining deterministic filesystem-device mismatch coverage.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/node/internal/workspace -run 'TestWorkspaceCleanup' -count=1`
|
||||
- Expected: exactly one cleanup result owns all registered target resources; suspect/unowned entries fail closed; no user/foreign request file, mounted content, or process is touched.
|
||||
|
||||
### [API-2] Complete typed cleanup handling and coordinator finalization
|
||||
|
||||
**Problem**
|
||||
|
||||
- Packet 10 leaves `OnWorkspaceCleanup` unsupported.
|
||||
- Packet 03's planned `finalizing` state waits only for endpoint acknowledgement, not workspace cleanup.
|
||||
|
||||
**Solution**
|
||||
|
||||
Before (packet 03 state contract):
|
||||
|
||||
```text
|
||||
successful candidate -> finalizing -> endpoint acknowledgement -> completed
|
||||
```
|
||||
|
||||
After:
|
||||
|
||||
```text
|
||||
successful candidate -> finalizing -> exactly-once workspace cleanup
|
||||
-> endpoint acknowledgement -> completed
|
||||
```
|
||||
|
||||
Implement Node cleanup mapping. Add a separate optional `SingleRequestWorkspaceLifecycle` interface implemented by packet 12's tool loop (`CleanupWorkspace`). The coordinator invokes it once on every terminal/cancel path if the workspace was opened and waits within the frozen request deadline. Success plus cleanup failure becomes failed; existing failure/cancel keeps its category while recording only a safe cleanup code. Caller disconnect still cancels and awaits cleanup even when no response can be written.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/node/internal/node/workspace_handler.go` — map typed cleanup request/result without raw fields.
|
||||
- [ ] `apps/node/internal/node/workspace_handler_test.go` — cleanup identity/status/idempotence and missing-runtime mapping.
|
||||
- [ ] `apps/edge/internal/service/single_request_tool_loop.go` — implement optional workspace lifecycle cleanup using packet 09 wire.
|
||||
- [ ] `apps/edge/internal/service/single_request.go` — exactly-once cleanup gate before terminal acknowledgement/return.
|
||||
- [ ] `apps/edge/internal/service/single_request_cleanup_test.go` — success/error/cancel/write-failure races, one wire call, cleanup failure, unopened workspace.
|
||||
- [ ] `agent-contract/inner/edge-node-runtime-wire.md` — define cleanup scope/idempotence/failure and preservation.
|
||||
- [ ] `agent-spec/runtime/edge-node-execution.md` — synchronize finalizing order and named evidence.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Use counting lifecycle fakes plus a net-pipe Node cleanup handler. Race terminal candidates/cancel and assert one cleanup, no early completed state, and correct terminal category.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/node/internal/node ./apps/edge/internal/service -run 'Test(NodeWorkspaceCleanup|SingleRequestCleanup)' -count=1`
|
||||
- `rg --sort path -n 'cleanup|\.iop/job|user result|finalizing|exactly' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
- Expected: Node and coordinator share the exact scoped cleanup/finalization contract.
|
||||
|
||||
## Modified Files Summary
|
||||
|
||||
| File | Item |
|
||||
|------|------|
|
||||
| `apps/node/internal/workspace/runtime.go` | API-1 |
|
||||
| `apps/node/internal/workspace/cleanup.go` | API-1 |
|
||||
| `apps/node/internal/workspace/cleanup_path_unix.go` | API-1 |
|
||||
| `apps/node/internal/workspace/cleanup_path_other.go` | API-1 |
|
||||
| `apps/node/internal/workspace/cleanup_test.go` | API-1 |
|
||||
| `apps/node/internal/node/workspace_handler.go` | API-2 |
|
||||
| `apps/node/internal/node/workspace_handler_test.go` | API-2 |
|
||||
| `apps/edge/internal/service/single_request_tool_loop.go` | API-2 |
|
||||
| `apps/edge/internal/service/single_request.go` | API-2 |
|
||||
| `apps/edge/internal/service/single_request_cleanup_test.go` | API-2 |
|
||||
| `agent-contract/inner/edge-node-runtime-wire.md` | API-2 |
|
||||
| `agent-spec/runtime/edge-node-execution.md` | API-2 |
|
||||
| `agent-task/m-iop-owned-single-request-agent-execution/13+12_workspace_cleanup/CODE_REVIEW-cloud-G10.md` | API-1, API-2 |
|
||||
|
||||
## Final Verification
|
||||
|
||||
1. `test -f agent-task/m-iop-owned-single-request-agent-execution/12+05,08,11_internal_tool_loop/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/12+05,08,11_internal_tool_loop/complete.log' | wc -l)" -eq 1`
|
||||
2. `go test -race ./apps/node/internal/workspace -run 'TestWorkspaceCleanup' -count=1`
|
||||
3. `go test -race ./apps/node/internal/node ./apps/edge/internal/service -run 'Test(NodeWorkspaceCleanup|SingleRequestCleanup)' -count=1`
|
||||
4. `go test ./apps/node/internal/workspace ./apps/node/internal/node ./apps/edge/internal/service -count=1`
|
||||
5. `go vet ./apps/node/internal/workspace ./apps/node/internal/node ./apps/edge/internal/service`
|
||||
6. `GOOS=darwin GOARCH=arm64 go test -c -o /tmp/iop-workspace-cleanup-darwin.test ./apps/node/internal/workspace`
|
||||
7. `rg --sort path -n 'cleanup|request_id|\.iop/job|inventory|no-follow|user result|finalizing|exactly' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
8. `git diff --check`
|
||||
|
||||
Expected: packet 12 is uniquely complete; cleanup is scoped to immutable request identity and inventoried Node artifacts, refuses symlink/mount/unowned replacement, is bounded and race-free, and precedes terminal completion; Darwin compile and package regressions pass. Cached tests are not acceptable.
|
||||
|
||||
**After completing all code changes, fill implementation-owned sections in `CODE_REVIEW-*-G??.md`.**
|
||||
|
|
@ -0,0 +1,162 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/13+12_workspace_cleanup plan=0 tag=API milestone-task=cleanup-observation -->
|
||||
|
||||
# Code Review Reference - API
|
||||
|
||||
> **[IMPLEMENTING AGENT — READ FIRST] Filling in this file is the mandatory final step of implementation.**
|
||||
> The task is NOT complete until every implementation-owned section below is filled in.
|
||||
> Complete the `Implementation Checklist`; the final checklist item is mandatory before saving.
|
||||
> Fill implementation-owned sections, then stop with active files in place and report ready for review.
|
||||
> Execute the plan's selected root cause, scope, files, and dependency decisions as written. Do not choose another owner, narrow/expand the write boundary, or replace a fix with another verification attempt.
|
||||
> If implementation is blocked, record the exact blocker, attempted commands/output, and resume condition only in implementation-owned evidence fields.
|
||||
> Do not ask the user directly, present choices, call user-input tools, create control-plane stop files, or classify the next state.
|
||||
> Finalization (`Code Review Result`, log rename, `complete.log`, archive moves, `Review-Only Checklist`) is review-agent-only, even after compaction/resume.
|
||||
> Follow the ownership table at the bottom of this file for which sections you own.
|
||||
|
||||
## Overview
|
||||
|
||||
date=2026-08-06
|
||||
task=m-iop-owned-single-request-agent-execution/13+12_workspace_cleanup, plan=0, tag=API
|
||||
|
||||
## For the Review Agent
|
||||
|
||||
> **[REVIEW AGENT ONLY]** The finalization steps below are review-agent only. Implementing agents must not execute this section.
|
||||
|
||||
Compare implementation of each item against source files and verify that output in `Verification Results` matches code.
|
||||
Review completion means the following steps are finished:
|
||||
|
||||
1. Append verdict and `review_rework_count` / `evidence_integrity_failure` routing signals.
|
||||
2. Archive `CODE_REVIEW-cloud-G10.md` → `code_review_cloud_G10_0.log` and `PLAN-cloud-G09.md` → `plan_cloud_G09_0.log`.
|
||||
3. If PASS, write `complete.log` and move active task directory to `agent-task/archive/YYYY/MM/m-iop-owned-single-request-agent-execution/13+12_workspace_cleanup/`. If WARN/FAIL, fully write the next filesystem state required by the code-review skill.
|
||||
4. If PASS, preserve the first-line `milestone-task=cleanup-observation` metadata in `complete.log` and report it for runtime aggregation. Roadmap state evaluation belongs to `sync-milestone-workstate`.
|
||||
5. Check applicable `Review-Only Checklist` items at the final `.log` location before reporting.
|
||||
|
||||
---
|
||||
|
||||
## Implementation Item Completion
|
||||
|
||||
| Item | Status |
|
||||
|------|---------|
|
||||
| API-1 Reclaim only Node request-owned state | [ ] |
|
||||
| API-2 Complete typed cleanup handling and coordinator finalization | [ ] |
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Create and validate only `.iop/job/<execution_id>` as the request-owned artifact namespace and preserve every user result outside it.
|
||||
- [ ] Cancel/wait all process groups and remove the job namespace exactly once per execution with bounded concurrent/idempotent result ownership.
|
||||
- [ ] Make coordinator success/error/cancel/disconnect paths converge on one typed cleanup before terminal commit, with fail-closed success handling.
|
||||
- [ ] Prove cleanup races, user-result preservation, cross-request isolation, failure handling, and synchronize cleanup contract/spec claims.
|
||||
- [ ] Run dependency, focused race, package, vet, Darwin compile, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Review-Only Checklist
|
||||
|
||||
> **[REVIEW AGENT ONLY]** Implementers must not modify/check this section.
|
||||
|
||||
- [ ] Append verdict, routing signals, dimensions, and findings.
|
||||
- [ ] Archive the active pair to routed suffix `0` logs and verify `.gitignore`.
|
||||
- [ ] On PASS write `complete.log`, preserve/report Milestone metadata, move this directory, and keep the active parent while siblings remain.
|
||||
- [ ] On WARN/FAIL write only the required next loop state.
|
||||
|
||||
## Deviations from Plan
|
||||
|
||||
_Record deviations and rationale._
|
||||
|
||||
## Key Design Decisions
|
||||
|
||||
_Record implemented decisions._
|
||||
|
||||
## Reviewer Checkpoints
|
||||
|
||||
- Confirm the only recursive target is the derived `.iop/job/<validated-id>` path through `os.Root`.
|
||||
- Confirm every process group for one execution is cancelled/waited and foreign request processes are untouched.
|
||||
- Confirm duplicate/racing cleanup shares one result without unbounded state growth.
|
||||
- Confirm final success waits for cleanup and cleanup failure cannot yield partial success.
|
||||
- Confirm user result files survive success, error, cancel, and runtime close.
|
||||
|
||||
## Verification Results
|
||||
|
||||
Paste actual stdout/stderr for every command; record replacements under deviations.
|
||||
|
||||
### 1. Dependency
|
||||
|
||||
`test -f agent-task/m-iop-owned-single-request-agent-execution/12+05,08,11_internal_tool_loop/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/12+05,08,11_internal_tool_loop/complete.log' | wc -l)" -eq 1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 2. Node cleanup race tests
|
||||
|
||||
`go test -race ./apps/node/internal/workspace -run 'TestWorkspaceCleanup' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 3. Handler/coordinator race tests
|
||||
|
||||
`go test -race ./apps/node/internal/node ./apps/edge/internal/service -run 'Test(NodeWorkspaceCleanup|SingleRequestCleanup)' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 4. Package regression
|
||||
|
||||
`go test ./apps/node/internal/workspace ./apps/node/internal/node ./apps/edge/internal/service -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 5. Vet
|
||||
|
||||
`go vet ./apps/node/internal/workspace ./apps/node/internal/node ./apps/edge/internal/service`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 6. Darwin compile
|
||||
|
||||
`GOOS=darwin GOARCH=arm64 go test -c -o /tmp/iop-workspace-cleanup-darwin.test ./apps/node/internal/workspace`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 7. Contract/spec search
|
||||
|
||||
`rg --sort path -n 'cleanup|\.iop/job|user result|finalizing|exactly' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 8. Whitespace
|
||||
|
||||
`git diff --check`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
> **[IMPLEMENTING AGENT — BEFORE SAVING] Have you filled in every implementation-owned section?**
|
||||
> If anything is blank, go back and fill it in before saving this file.
|
||||
> Leave review-agent-only sections unchanged.
|
||||
|
||||
## Section Ownership
|
||||
|
||||
| Section | Owner | Note |
|
||||
|---------|-------|------|
|
||||
| Header comment, Overview, Review Agent Instructions | Fixed at stub creation | Implementing agent must not modify or execute these (archive, complete.log, and task-directory archive move are review-agent only) |
|
||||
| Archive Evidence Snapshot | Fixed at stub creation from plan when present | Implementing agent uses it as default prior-loop context; read only the specific archive files cited there when more detail is required |
|
||||
| Implementation Item Completion (item names) | Fixed at stub creation | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Implementation Checklist (item text/order) | Fixed at stub creation from plan | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Review-Only Checklist | Review agent only | Implementing agent must not modify or check this section |
|
||||
| Deviations from Plan, Key Design Decisions | Implementing agent | Replace placeholder text with actual content |
|
||||
| Reviewer Checkpoints | Fixed at stub creation | Pre-filled from plan |
|
||||
| Verification Results (section headings + commands) | Fixed at stub creation | Implementing agent fills in command output only; command changes require a `Deviations from Plan` entry |
|
||||
| Code Review Result | Review agent appends | Not included in stub |
|
||||
|
|
@ -0,0 +1,196 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/13+12_workspace_cleanup plan=0 tag=API milestone-task=cleanup-observation -->
|
||||
|
||||
# Request-owned Workspace Cleanup
|
||||
|
||||
## For the Implementing Agent
|
||||
|
||||
Do not start until packet 12 has `complete.log`. Implement only request-owned process/artifact cleanup in the listed files, preserve user results, run every verification command, and fill `CODE_REVIEW-cloud-G10.md`. Do not broaden cleanup into rollback or own official review state.
|
||||
|
||||
## Background
|
||||
|
||||
The internal loop can open a Node workspace and run processes, but success/error/cancel paths do not yet converge on one cleanup owner. This packet adds exactly-once coordinator cleanup and Node reclamation limited to active command groups and `.iop/job/<execution_id>`.
|
||||
|
||||
## Analysis
|
||||
|
||||
### Files Read
|
||||
|
||||
- `AGENTS.md`
|
||||
- `agent-ops/rules/project/rules.md`
|
||||
- `agent-ops/rules/common/rules-roadmap.md`
|
||||
- `agent-ops/rules/common/rules-agent-spec.md`
|
||||
- `agent-ops/rules/project/domain/edge/rules.md`
|
||||
- `agent-ops/rules/project/domain/node/rules.md`
|
||||
- `agent-test/local/rules.md`
|
||||
- `agent-test/local/edge-smoke.md`
|
||||
- `agent-test/local/node-smoke.md`
|
||||
- `agent-roadmap/phase/knowledge-tool-optimization-extension/milestones/iop-owned-single-request-agent-execution.md`
|
||||
- `agent-roadmap/sdd/knowledge-tool-optimization-extension/iop-owned-single-request-agent-execution/SDD.md`
|
||||
- `agent-task/m-iop-owned-single-request-agent-execution/03+02_single_request_coordinator/PLAN-local-G07.md`
|
||||
- `apps/edge/internal/service/service.go`
|
||||
- `apps/node/internal/node/cancel_handler.go`
|
||||
- `apps/node/internal/node/node.go`
|
||||
- `agent-contract/inner/edge-node-runtime-wire.md`
|
||||
- `agent-spec/runtime/edge-node-execution.md`
|
||||
|
||||
### SDD Criteria
|
||||
|
||||
- S07 requires success, error, and cancel to remove request-owned processes and `.iop/job` artifacts while preserving requested workspace results.
|
||||
- `finalizing` waits for cleanup before committing the final terminal. Cleanup never rolls back user files.
|
||||
- S11 caller disconnect must cancel provider/tool processes and perform bounded cleanup without another external request.
|
||||
|
||||
### Verification Context
|
||||
|
||||
- Packet 09 already defines typed cleanup; packets 10/11 own request contexts and process groups; packet 12 owns the coordinator loop.
|
||||
- Race tests use temporary roots and helper processes. No external Mac runner is required for logic, while Darwin compile remains a final gate.
|
||||
- Existing route-01 cleanup is caller-artifact state and is not reusable as Node filesystem ownership.
|
||||
|
||||
### State and Concurrency Findings
|
||||
|
||||
- Success, executor error, request timeout, caller cancel, terminal write failure, and duplicate callbacks may all race to cleanup; only one wire cleanup is sent and all callers observe its result.
|
||||
- Node cleanup cancels every active process for exactly one execution, waits boundedly, removes only the safe internal job directory, closes the request context, and caches a bounded idempotent result.
|
||||
- Cleanup failure converts a pending success to failure; existing failure/cancel remains primary while safe cleanup failure is internal evidence.
|
||||
|
||||
### Test Coverage Gaps
|
||||
|
||||
- No Node operation removes request job artifacts or all request process groups.
|
||||
- Coordinator finalization does not wait for workspace cleanup or prove exactly-once across terminal races.
|
||||
|
||||
### Symbol References
|
||||
|
||||
- Add an optional lifecycle interface to packet 12's internal tool executor; do not break fakes that never opened a workspace.
|
||||
- Provider `CancelRun` and route-01 cleanup remain separate.
|
||||
|
||||
### Split Judgment
|
||||
|
||||
- Node cleanup and coordinator exactly-once finalization form one end-to-end ownership invariant and must be reviewed together.
|
||||
- Observation is packet 14 because it can instrument the stable lifecycle without changing cleanup outcomes.
|
||||
|
||||
### Scope Rationale
|
||||
|
||||
- Include job namespace, process cancellation/wait, idempotence, coordinator deferral, user-file preservation, tests, wire contract/spec.
|
||||
- Exclude general rollback, git reset, arbitrary directory cleanup, metrics/logging implementation, and real provider smoke.
|
||||
|
||||
### Final Routing
|
||||
|
||||
- `evaluation_mode=first-pass`; build closures true, scores 2/2/2/1/2 = G09.
|
||||
- Finalizer route `grade-boundary`, lane `cloud`, filename `PLAN-cloud-G09.md`; risks `temporal_state`, `concurrent_consistency`, `boundary_contract`, `variant_product` (4).
|
||||
- Review scores 2/2/2/2/2 = G10; official filename `CODE_REVIEW-cloud-G10.md`; no capability/recovery gap.
|
||||
|
||||
## Dependencies and Execution Order
|
||||
|
||||
1. Require packet 12 completion.
|
||||
2. Implement Node cleanup and race tests.
|
||||
3. Add optional Edge lifecycle cleanup and terminal ordering.
|
||||
4. Synchronize contract/spec only after end-to-end verification.
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Create and validate only `.iop/job/<execution_id>` as the request-owned artifact namespace and preserve every user result outside it.
|
||||
- [ ] Cancel/wait all process groups and remove the job namespace exactly once per execution with bounded concurrent/idempotent result ownership.
|
||||
- [ ] Make coordinator success/error/cancel/disconnect paths converge on one typed cleanup before terminal commit, with fail-closed success handling.
|
||||
- [ ] Prove cleanup races, user-result preservation, cross-request isolation, failure handling, and synchronize cleanup contract/spec claims.
|
||||
- [ ] Run dependency, focused race, package, vet, Darwin compile, documentation, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Implementation Plan
|
||||
|
||||
### [API-1] Reclaim only Node request-owned state
|
||||
|
||||
**Problem**
|
||||
|
||||
- Packet 11 tracks commands per execution but has no all-process cleanup or artifact namespace reclamation.
|
||||
- `apps/node/internal/node/cancel_handler.go:11` owns provider run cancel only.
|
||||
|
||||
**Solution**
|
||||
|
||||
Add `cleanup.go` to workspace runtime. On request open, create `.iop/job/<validated-execution-id>` through `os.Root`; never derive it from a caller path. Cleanup atomically elects one owner, cancels all process groups for that execution, waits up to the admitted cleanup bound, then `Root.RemoveAll` on that exact internal relative path and closes the request context. Concurrent/duplicate callers wait for and receive the same typed result. Retain completed results in a bounded cache; eviction may repeat an idempotent missing-directory check but never broadens scope. Runtime close invokes the same primitive for active requests.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/node/internal/workspace/runtime.go` — request lifecycle state and bounded completed-cleanup cache.
|
||||
- [ ] `apps/node/internal/workspace/cleanup.go` — process cancel/wait, exact job path removal, idempotent result.
|
||||
- [ ] `apps/node/internal/workspace/cleanup_test.go` — success/error/cancel races, duplicate calls, active child, timeout, user-result/cross-request preservation, invalid id.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Create user results beside `.iop`, two executions, nested job artifacts, and helper process descendants; race cleanup/cancel/close and assert only the target job path disappears.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/node/internal/workspace -run 'TestWorkspaceCleanup' -count=1`
|
||||
- Expected: exactly one cleanup result owns all target resources and no user/foreign request file or process is touched.
|
||||
|
||||
### [API-2] Complete typed cleanup handling and coordinator finalization
|
||||
|
||||
**Problem**
|
||||
|
||||
- Packet 10 leaves `OnWorkspaceCleanup` unsupported.
|
||||
- Packet 03's planned `finalizing` state waits only for endpoint acknowledgement, not workspace cleanup.
|
||||
|
||||
**Solution**
|
||||
|
||||
Before (packet 03 state contract):
|
||||
|
||||
```text
|
||||
successful candidate -> finalizing -> endpoint acknowledgement -> completed
|
||||
```
|
||||
|
||||
After:
|
||||
|
||||
```text
|
||||
successful candidate -> finalizing -> exactly-once workspace cleanup
|
||||
-> endpoint acknowledgement -> completed
|
||||
```
|
||||
|
||||
Implement Node cleanup mapping. Add a separate optional `SingleRequestWorkspaceLifecycle` interface implemented by packet 12's tool loop (`CleanupWorkspace`). The coordinator invokes it once on every terminal/cancel path if the workspace was opened and waits within the frozen request deadline. Success plus cleanup failure becomes failed; existing failure/cancel keeps its category while recording only a safe cleanup code. Caller disconnect still cancels and awaits cleanup even when no response can be written.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/node/internal/node/workspace_handler.go` — map typed cleanup request/result without raw fields.
|
||||
- [ ] `apps/node/internal/node/workspace_handler_test.go` — cleanup identity/status/idempotence and missing-runtime mapping.
|
||||
- [ ] `apps/edge/internal/service/single_request_tool_loop.go` — implement optional workspace lifecycle cleanup using packet 09 wire.
|
||||
- [ ] `apps/edge/internal/service/single_request.go` — exactly-once cleanup gate before terminal acknowledgement/return.
|
||||
- [ ] `apps/edge/internal/service/single_request_cleanup_test.go` — success/error/cancel/write-failure races, one wire call, cleanup failure, unopened workspace.
|
||||
- [ ] `agent-contract/inner/edge-node-runtime-wire.md` — define cleanup scope/idempotence/failure and preservation.
|
||||
- [ ] `agent-spec/runtime/edge-node-execution.md` — synchronize finalizing order and named evidence.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Use counting lifecycle fakes plus a net-pipe Node cleanup handler. Race terminal candidates/cancel and assert one cleanup, no early completed state, and correct terminal category.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/node/internal/node ./apps/edge/internal/service -run 'Test(NodeWorkspaceCleanup|SingleRequestCleanup)' -count=1`
|
||||
- `rg --sort path -n 'cleanup|\.iop/job|user result|finalizing|exactly' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
- Expected: Node and coordinator share the exact scoped cleanup/finalization contract.
|
||||
|
||||
## Modified Files Summary
|
||||
|
||||
| File | Item |
|
||||
|------|------|
|
||||
| `apps/node/internal/workspace/runtime.go` | API-1 |
|
||||
| `apps/node/internal/workspace/cleanup.go` | API-1 |
|
||||
| `apps/node/internal/workspace/cleanup_test.go` | API-1 |
|
||||
| `apps/node/internal/node/workspace_handler.go` | API-2 |
|
||||
| `apps/node/internal/node/workspace_handler_test.go` | API-2 |
|
||||
| `apps/edge/internal/service/single_request_tool_loop.go` | API-2 |
|
||||
| `apps/edge/internal/service/single_request.go` | API-2 |
|
||||
| `apps/edge/internal/service/single_request_cleanup_test.go` | API-2 |
|
||||
| `agent-contract/inner/edge-node-runtime-wire.md` | API-2 |
|
||||
| `agent-spec/runtime/edge-node-execution.md` | API-2 |
|
||||
| `agent-task/m-iop-owned-single-request-agent-execution/13+12_workspace_cleanup/CODE_REVIEW-cloud-G10.md` | API-1, API-2 |
|
||||
|
||||
## Final Verification
|
||||
|
||||
1. `test -f agent-task/m-iop-owned-single-request-agent-execution/12+05,08,11_internal_tool_loop/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/12+05,08,11_internal_tool_loop/complete.log' | wc -l)" -eq 1`
|
||||
2. `go test -race ./apps/node/internal/workspace -run 'TestWorkspaceCleanup' -count=1`
|
||||
3. `go test -race ./apps/node/internal/node ./apps/edge/internal/service -run 'Test(NodeWorkspaceCleanup|SingleRequestCleanup)' -count=1`
|
||||
4. `go test ./apps/node/internal/workspace ./apps/node/internal/node ./apps/edge/internal/service -count=1`
|
||||
5. `go vet ./apps/node/internal/workspace ./apps/node/internal/node ./apps/edge/internal/service`
|
||||
6. `GOOS=darwin GOARCH=arm64 go test -c -o /tmp/iop-workspace-cleanup-darwin.test ./apps/node/internal/workspace`
|
||||
7. `rg --sort path -n 'cleanup|\.iop/job|user result|finalizing|exactly' agent-contract/inner/edge-node-runtime-wire.md agent-spec/runtime/edge-node-execution.md`
|
||||
8. `git diff --check`
|
||||
|
||||
Expected: packet 12 is uniquely complete; cleanup is scoped, bounded, race-free, and precedes terminal completion; Darwin compile and package regressions pass. Cached tests are not acceptable.
|
||||
|
||||
**After completing all code changes, fill implementation-owned sections in `CODE_REVIEW-*-G??.md`.**
|
||||
|
|
@ -0,0 +1,172 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/14+05,12,13_workspace_observation plan=0 tag=API milestone-task=cleanup-observation -->
|
||||
|
||||
# Code Review Reference - API
|
||||
|
||||
> **[IMPLEMENTING AGENT — READ FIRST] Filling in this file is the mandatory final step of implementation.**
|
||||
> The task is NOT complete until every implementation-owned section below is filled in.
|
||||
> Complete the `Implementation Checklist`; the final checklist item is mandatory before saving.
|
||||
> Fill implementation-owned sections, then stop with active files in place and report ready for review.
|
||||
> Execute the plan's selected root cause, scope, files, and dependency decisions as written. Do not choose another owner, narrow/expand the write boundary, or replace a fix with another verification attempt.
|
||||
> If implementation is blocked, record the exact blocker, attempted commands/output, and resume condition only in implementation-owned evidence fields.
|
||||
> Do not ask the user directly, present choices, call user-input tools, create control-plane stop files, or classify the next state.
|
||||
> Finalization (`Code Review Result`, log rename, `complete.log`, archive moves, `Review-Only Checklist`) is review-agent-only, even after compaction/resume.
|
||||
> Follow the ownership table at the bottom of this file for which sections you own.
|
||||
|
||||
## Overview
|
||||
|
||||
date=2026-08-06
|
||||
task=m-iop-owned-single-request-agent-execution/14+05,12,13_workspace_observation, plan=0, tag=API
|
||||
|
||||
## For the Review Agent
|
||||
|
||||
> **[REVIEW AGENT ONLY]** The finalization steps below are review-agent only. Implementing agents must not execute this section.
|
||||
|
||||
Compare implementation of each item against source files and verify that output in `Verification Results` matches code.
|
||||
Review completion means the following steps are finished:
|
||||
|
||||
1. Append verdict and `review_rework_count` / `evidence_integrity_failure` routing signals.
|
||||
2. Archive `CODE_REVIEW-cloud-G08.md` → `code_review_cloud_G08_0.log` and `PLAN-cloud-G07.md` → `plan_cloud_G07_0.log`.
|
||||
3. If PASS, write `complete.log` and move active task directory to `agent-task/archive/YYYY/MM/m-iop-owned-single-request-agent-execution/14+05,12,13_workspace_observation/`. If WARN/FAIL, fully write the next filesystem state required by the code-review skill.
|
||||
4. If PASS, preserve the first-line `milestone-task=cleanup-observation` metadata in `complete.log` and report it for runtime aggregation. Roadmap state evaluation belongs to `sync-milestone-workstate`.
|
||||
5. Check applicable `Review-Only Checklist` items at the final `.log` location before reporting.
|
||||
|
||||
---
|
||||
|
||||
## Implementation Item Completion
|
||||
|
||||
| Item | Status |
|
||||
|------|---------|
|
||||
| API-1 Define closed observation and timing semantics | [ ] |
|
||||
| API-2 Emit bounded Edge metrics/logs and safe Node events | [ ] |
|
||||
| API-3 Link ingress, lifecycle, and documented evidence | [ ] |
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Define a closed, copy-safe single-request observation schema and explicit log/metric allowlists that exclude all raw or unbounded values.
|
||||
- [ ] Measure request total, provider-active stage, Node tool, and cleanup durations/outcomes exactly once without counting tool time as stage pure time.
|
||||
- [ ] Add failure-isolated bounded Prometheus/zap observers, wire them at Edge startup, and emit raw-free Node tool/cleanup logs.
|
||||
- [ ] Prove cardinality, correlation, timing math, terminal races, observer panic/error isolation, secret sentinels, and ingress-to-total count consistency.
|
||||
- [ ] Synchronize input/runtime specs with deterministic evidence and explicit external Claude/Mac smoke deferral.
|
||||
- [ ] Run all dependency, focused race, package, vet, deterministic search, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Review-Only Checklist
|
||||
|
||||
> **[REVIEW AGENT ONLY]** Implementers must not modify/check this section.
|
||||
|
||||
- [ ] Append verdict, routing signals, dimensions, and findings.
|
||||
- [ ] Archive the active pair to routed suffix `0` logs and verify `.gitignore`.
|
||||
- [ ] On PASS write `complete.log`, preserve/report Milestone metadata, move this directory, and keep the parent while siblings remain.
|
||||
- [ ] On WARN/FAIL write only the official next loop state.
|
||||
|
||||
## Deviations from Plan
|
||||
|
||||
_Record deviations and rationale._
|
||||
|
||||
## Key Design Decisions
|
||||
|
||||
_Record implemented decisions._
|
||||
|
||||
## Reviewer Checkpoints
|
||||
|
||||
- Confirm metric labels are closed/bounded and omit every request/stage/tool id and raw value.
|
||||
- Confirm log keys are exact allowlists and sanitizer tests include path/command/output/credential sentinels.
|
||||
- Confirm stage pure time pauses across tools, total includes cleanup/terminal resolution, and each terminal emits once.
|
||||
- Confirm observer error/panic cannot alter response, cancellation, cleanup, or process ownership.
|
||||
- Confirm Node and Edge events correlate safely and external Claude/Mac smoke remains unclaimed.
|
||||
|
||||
## Verification Results
|
||||
|
||||
Paste actual stdout/stderr for every command; record replacements under deviations.
|
||||
|
||||
### 1. Packet 05 dependency
|
||||
|
||||
`test -f agent-task/m-iop-owned-single-request-agent-execution/05+03_single_ingress/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/05+03_single_ingress/complete.log' | wc -l)" -eq 1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 2. Packet 12 dependency
|
||||
|
||||
`test -f agent-task/m-iop-owned-single-request-agent-execution/12+05,08,11_internal_tool_loop/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/12+05,08,11_internal_tool_loop/complete.log' | wc -l)" -eq 1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 3. Packet 13 dependency
|
||||
|
||||
`test -f agent-task/m-iop-owned-single-request-agent-execution/13+12_workspace_cleanup/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/13+12_workspace_cleanup/complete.log' | wc -l)" -eq 1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 4. Focused race tests
|
||||
|
||||
`go test -race ./apps/edge/internal/service ./apps/edge/internal/bootstrap ./apps/node/internal/workspace -run 'Test(SingleRequestObservation|SingleRequestMetrics|SingleRequestObservationWiring|WorkspaceObservation)' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 5. HTTP evidence
|
||||
|
||||
`go test ./apps/edge/internal/openai -run 'TestAnthropicSingleRequestObservation' -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 6. Package regression
|
||||
|
||||
`go test ./apps/edge/internal/service ./apps/edge/internal/bootstrap ./apps/edge/internal/openai ./apps/node/internal/workspace -count=1`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 7. Vet
|
||||
|
||||
`go vet ./apps/edge/internal/service ./apps/edge/internal/bootstrap ./apps/edge/internal/openai ./apps/node/internal/workspace`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 8. Spec search
|
||||
|
||||
`rg --sort path -n 'stage.*pure|tool.*duration|cleanup|total|raw|cardinality|Claude.*defer' agent-spec/input/openai-compatible-surface.md agent-spec/runtime/edge-node-execution.md`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
### 9. Whitespace
|
||||
|
||||
`git diff --check`
|
||||
|
||||
```text
|
||||
[fill]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
> **[IMPLEMENTING AGENT — BEFORE SAVING] Have you filled in every implementation-owned section?**
|
||||
> If anything is blank, go back and fill it in before saving this file.
|
||||
> Leave review-agent-only sections unchanged.
|
||||
|
||||
## Section Ownership
|
||||
|
||||
| Section | Owner | Note |
|
||||
|---------|-------|------|
|
||||
| Header comment, Overview, Review Agent Instructions | Fixed at stub creation | Implementing agent must not modify or execute these (archive, complete.log, and task-directory archive move are review-agent only) |
|
||||
| Archive Evidence Snapshot | Fixed at stub creation from plan when present | Implementing agent uses it as default prior-loop context; read only the specific archive files cited there when more detail is required |
|
||||
| Implementation Item Completion (item names) | Fixed at stub creation | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Implementation Checklist (item text/order) | Fixed at stub creation from plan | Implementing agent checks `[ ]` → `[x]` only |
|
||||
| Review-Only Checklist | Review agent only | Implementing agent must not modify or check this section |
|
||||
| Deviations from Plan, Key Design Decisions | Implementing agent | Replace placeholder text with actual content |
|
||||
| Reviewer Checkpoints | Fixed at stub creation | Pre-filled from plan |
|
||||
| Verification Results (section headings + commands) | Fixed at stub creation | Implementing agent fills in command output only; command changes require a `Deviations from Plan` entry |
|
||||
| Code Review Result | Review agent appends | Not included in stub |
|
||||
|
|
@ -0,0 +1,229 @@
|
|||
<!-- task=m-iop-owned-single-request-agent-execution/14+05,12,13_workspace_observation plan=0 tag=API milestone-task=cleanup-observation -->
|
||||
|
||||
# Raw-free Single-request Timing Observation
|
||||
|
||||
## For the Implementing Agent
|
||||
|
||||
Do not start until packets 05, 12, and 13 each have `complete.log`. Instrument the stable lifecycle without changing request outcomes, use only closed fields/labels, run every command, and fill `CODE_REVIEW-cloud-G08.md`. Review finalization and any external smoke remain outside this packet.
|
||||
|
||||
## Background
|
||||
|
||||
Ingress count, internal tools, and cleanup will exist, but S07 still needs linked request/stage/tool/cleanup/total timing and outcomes without raw prompt, path, command, output, credential, provider, or unbounded metric labels. This packet adds failure-isolated Edge metrics/log projection and Node tool logs over the completed lifecycle.
|
||||
|
||||
## Analysis
|
||||
|
||||
### Files Read
|
||||
|
||||
- `AGENTS.md`
|
||||
- `agent-ops/rules/project/rules.md`
|
||||
- `agent-ops/rules/common/rules-roadmap.md`
|
||||
- `agent-ops/rules/common/rules-agent-spec.md`
|
||||
- `agent-ops/rules/project/domain/edge/rules.md`
|
||||
- `agent-ops/rules/project/domain/node/rules.md`
|
||||
- `agent-ops/rules/project/domain/platform-common/rules.md`
|
||||
- `agent-test/local/rules.md`
|
||||
- `agent-test/local/edge-smoke.md`
|
||||
- `agent-test/local/node-smoke.md`
|
||||
- `agent-roadmap/phase/knowledge-tool-optimization-extension/milestones/iop-owned-single-request-agent-execution.md`
|
||||
- `agent-roadmap/sdd/knowledge-tool-optimization-extension/iop-owned-single-request-agent-execution/SDD.md`
|
||||
- `apps/edge/internal/service/service.go`
|
||||
- `apps/edge/internal/bootstrap/runtime.go`
|
||||
- `apps/edge/internal/input/manager.go`
|
||||
- `apps/edge/internal/openai/server.go`
|
||||
- `apps/edge/internal/openai/hot_path_observation.go`
|
||||
- `apps/edge/internal/openai/hot_path_metrics.go`
|
||||
- `packages/go/observability/observability.go`
|
||||
- `agent-spec/input/openai-compatible-surface.md`
|
||||
- `agent-spec/runtime/edge-node-execution.md`
|
||||
|
||||
### SDD Criteria
|
||||
|
||||
- S07 requires raw-free request/stage/tool/total timing and outcome linked across success/error/cancel, alongside cleanup preservation evidence.
|
||||
- S12 later requires actual Claude/Mac evidence; this packet supplies runtime instrumentation and deterministic synthetic evidence only.
|
||||
- D10 forbids internal reasoning/tool protocol in outer output; observation must also exclude raw command/path/output and credentials.
|
||||
|
||||
### Verification Context
|
||||
|
||||
- Existing Hot Path observation demonstrates closed enums, explicit log-key allowlists, bounded Prometheus labels, injected sinks, and failure isolation; single-request metrics remain a separate owner.
|
||||
- Packet 05 supplies a no-label ingress counter. Packets 12/13 supply stable lifecycle seams and Node response durations.
|
||||
- Baseline packages passed fresh; deterministic tests use an injected clock and Prometheus gatherer, no external runner.
|
||||
|
||||
### State and Concurrency Findings
|
||||
|
||||
- Total wall time begins at accepted marked admission and ends after cleanup plus terminal acknowledgement/outcome.
|
||||
- Stage pure time accumulates only provider-active intervals; it pauses during `internal_tool` Node execution and resumes on result continuation. Tool duration comes from typed Node execution and cleanup has its own interval.
|
||||
- Observer panic/error must not change execution or terminal. One terminal event wins the same coordinator race as the terminal itself.
|
||||
|
||||
### Test Coverage Gaps
|
||||
|
||||
- Packet 05 observes ingress only; no stage/tool/cleanup/total collectors or linked log schema exists.
|
||||
- Node workspace results have duration fields but no closed raw-free local observation seam.
|
||||
|
||||
### Symbol References
|
||||
|
||||
- Do not merge with route-01 `hotPathObserver` or Stream Gate observation; use distinct metric names/types.
|
||||
- Extend packet 05's `single_request_metrics.go` only for the existing ingress test accessor/correlation assertions; service owns lifecycle metrics.
|
||||
- No API/wire/config schema changes are required.
|
||||
|
||||
### Split Judgment
|
||||
|
||||
- Observation is independently PASS-capable after lifecycle completion and cannot alter cleanup or terminal semantics.
|
||||
- Actual Claude/Mac smoke remains the separate `claude-smoke` Task because credentials/device evidence is external.
|
||||
|
||||
### Scope Rationale
|
||||
|
||||
- Include closed event DTO, injected clock/observer, service metrics/zap adapter, Node safe logs, bootstrap wiring, allowlist/cardinality/failure tests, living specs.
|
||||
- Exclude raw payload logging, request-derived metric labels, dashboards/ledger, external smoke, and semantic outcome changes.
|
||||
|
||||
### Final Routing
|
||||
|
||||
- `evaluation_mode=first-pass`; build closures true, scores 2/1/1/1/2 = G07.
|
||||
- Finalizer route `risk-boundary`, lane `cloud`, filename `PLAN-cloud-G07.md`; risks `temporal_state`, `concurrent_consistency`, `boundary_contract`, `variant_product` (4).
|
||||
- Review scores 2/1/1/2/2 = G08; official filename `CODE_REVIEW-cloud-G08.md`; no capability/recovery gap.
|
||||
|
||||
## Dependencies and Execution Order
|
||||
|
||||
1. Require packet 05 ingress metric/HTTP evidence.
|
||||
2. Require packet 12 tool loop and packet 13 cleanup lifecycle.
|
||||
3. Define closed events and tests, instrument service/Node, wire production observer, then update specs.
|
||||
|
||||
## Implementation Checklist
|
||||
|
||||
- [ ] Define a closed, copy-safe single-request observation schema and explicit log/metric allowlists that exclude all raw or unbounded values.
|
||||
- [ ] Measure request total, provider-active stage, Node tool, and cleanup durations/outcomes exactly once without counting tool time as stage pure time.
|
||||
- [ ] Add failure-isolated bounded Prometheus/zap observers, wire them at Edge startup, and emit raw-free Node tool/cleanup logs.
|
||||
- [ ] Prove cardinality, correlation, timing math, terminal races, observer panic/error isolation, secret sentinels, and ingress-to-total count consistency.
|
||||
- [ ] Synchronize input/runtime specs with deterministic evidence and explicit external Claude/Mac smoke deferral.
|
||||
- [ ] Run all dependency, focused race, package, vet, deterministic search, and whitespace verification.
|
||||
- [ ] Fill implementation-owned sections in CODE_REVIEW-*-G??.md with actual implementation notes and verification output.
|
||||
|
||||
## Implementation Plan
|
||||
|
||||
### [API-1] Define closed observation and timing semantics
|
||||
|
||||
**Problem**
|
||||
|
||||
- `apps/edge/internal/service/service.go:28` has no observer or clock for packet 03/12/13 lifecycle.
|
||||
- `apps/edge/internal/openai/hot_path_observation.go:1` is route-01-specific and includes different states/identities.
|
||||
|
||||
**Solution**
|
||||
|
||||
Add service-owned closed enums for event class (`request`, `stage`, `tool`, `cleanup`, `terminal`), stage (`plan`, `work`, `review`), operation, and outcome/error class. The DTO may include a bounded generated execution correlation id in logs, but metric labels are only fixed Edge id plus closed enums. It contains durations/counts/truncated booleans, never request text, public model, provider id, Node/root/path, command/template/env, tool input/output, error string, header, credential, or raw terminal.
|
||||
|
||||
Add an injectable clock and failure-isolated observer snapshot on `Service`. Accumulate provider-active stage intervals around `internal_tool`, record Node tool duration once, cleanup once, terminal once, and request total after acknowledgement/cancel resolution. Unknown enum/value normalizes to empty and is dropped.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/edge/internal/service/service.go` — configure/snapshot observer and clock safely.
|
||||
- [ ] `apps/edge/internal/service/single_request_observation.go` — closed DTO/enums, allowlists, sanitizer, timer accumulation, safe emit.
|
||||
- [ ] `apps/edge/internal/service/single_request.go` — request/stage/terminal/total timing hooks.
|
||||
- [ ] `apps/edge/internal/service/single_request_tool_loop.go` — tool/pause/resume timing hooks.
|
||||
- [ ] `apps/edge/internal/service/single_request_observation_test.go` — deterministic clock, success/error/cancel/race, pure-time math, schema/sentinel, observer failure.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Use a manual clock and capturing/panicking observer. Assert exact event count/order and `stage_active + tool + cleanup <= total` with tool duration excluded from stage active time.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/edge/internal/service -run 'TestSingleRequestObservation' -count=1`
|
||||
- Expected: timing/event ownership is deterministic and observer failure never changes terminal behavior.
|
||||
|
||||
### [API-2] Emit bounded Edge metrics/logs and safe Node events
|
||||
|
||||
**Problem**
|
||||
|
||||
- Packet 05's planned `single_request_metrics.go` contains only `iop_anthropic_single_request_ingress_total`.
|
||||
- Node workspace runtime has no allowlisted outcome log projection.
|
||||
|
||||
**Solution**
|
||||
|
||||
Create service lifecycle Prometheus histograms/counters with closed labels and a zap observer with an exact key allowlist. Register once, normalize Edge id, and expose test-only gather helpers without request-derived labels. In Edge bootstrap, install this observer on the service before input servers are created. Extend the packet 05 test metric helper only as needed to compare ingress and request-total deltas.
|
||||
|
||||
Add a workspace observer at the single `Runtime.Execute`/cleanup completion seams. Its exact zap fields are execution correlation, operation, closed outcome/error code, duration, truncation, process/artifact counts; never path/content/command/env/stdout/stderr. Observer failure is swallowed.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/edge/internal/service/single_request_metrics.go` — bounded collectors and production zap/metric observer.
|
||||
- [ ] `apps/edge/internal/service/single_request_metrics_test.go` — collector labels/cardinality, exact deltas, duplicate-terminal protection.
|
||||
- [ ] `apps/edge/internal/bootstrap/runtime.go` — install the production observer before request handling.
|
||||
- [ ] `apps/edge/internal/bootstrap/single_request_observation_test.go` — assert wiring and failure isolation.
|
||||
- [ ] `apps/edge/internal/openai/single_request_metrics.go` — preserve ingress owner and expose bounded test correlation only.
|
||||
- [ ] `apps/node/internal/workspace/observation.go` — raw-free Node event projection and safe zap observer.
|
||||
- [ ] `apps/node/internal/workspace/runtime.go` — emit one tool event at the common completion seam.
|
||||
- [ ] `apps/node/internal/workspace/cleanup.go` — emit one cleanup event.
|
||||
- [ ] `apps/node/internal/workspace/observation_test.go` — exact key allowlist, sentinel rejection, outcome count, observer panic/error.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Gather metrics before/after deterministic flows and inspect captured zap cores. Assert no forbidden keys/values and no request id in metric labels.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test -race ./apps/edge/internal/service ./apps/edge/internal/bootstrap ./apps/node/internal/workspace -run 'Test(SingleRequestMetrics|SingleRequestObservationWiring|WorkspaceObservation)' -count=1`
|
||||
- Expected: bounded metrics/logs emit once and cannot influence runtime outcomes.
|
||||
|
||||
### [API-3] Link ingress, lifecycle, and documented evidence
|
||||
|
||||
**Problem**
|
||||
|
||||
- S07 evidence needs one linked synthetic flow and specs describing pure-time/cardinality/privacy semantics; current living specs have only generic/route-01 observation.
|
||||
|
||||
**Solution**
|
||||
|
||||
Extend packet 12's real-POST test to snapshot ingress and lifecycle metrics, run multiple stages/tools plus cleanup, and assert deltas: ingress=1, request-total=1, terminal=1, expected stage/tool/cleanup counts. Capture logs using the safe generated correlation id and verify forbidden sentinels are absent. Keep actual Claude/Mac timing evidence explicitly deferred to `claude-smoke`.
|
||||
|
||||
**Modified Files and Checklist**
|
||||
|
||||
- [ ] `apps/edge/internal/openai/single_request_handler_test.go` — ingress/lifecycle delta and raw-free correlation integration assertions.
|
||||
- [ ] `agent-spec/input/openai-compatible-surface.md` — document ingress/terminal correlation and privacy.
|
||||
- [ ] `agent-spec/runtime/edge-node-execution.md` — document stage-pure/tool/cleanup/total timing, labels, Node logs, and external-smoke deferral.
|
||||
|
||||
**Test Strategy**
|
||||
|
||||
- Add/extend `TestAnthropicSingleRequestObservation` using the real marked POST fixture and deterministic internal tools.
|
||||
|
||||
**Verification**
|
||||
|
||||
- `go test ./apps/edge/internal/openai -run 'TestAnthropicSingleRequestObservation' -count=1`
|
||||
- `rg --sort path -n 'stage.*pure|tool.*duration|cleanup|total|raw|cardinality|Claude.*defer' agent-spec/input/openai-compatible-surface.md agent-spec/runtime/edge-node-execution.md`
|
||||
- Expected: one synthetic request links all closed timing/outcome evidence and docs do not claim external smoke.
|
||||
|
||||
## Modified Files Summary
|
||||
|
||||
| File | Item |
|
||||
|------|------|
|
||||
| `apps/edge/internal/service/service.go` | API-1 |
|
||||
| `apps/edge/internal/service/single_request_observation.go` | API-1 |
|
||||
| `apps/edge/internal/service/single_request.go` | API-1 |
|
||||
| `apps/edge/internal/service/single_request_tool_loop.go` | API-1 |
|
||||
| `apps/edge/internal/service/single_request_observation_test.go` | API-1 |
|
||||
| `apps/edge/internal/service/single_request_metrics.go` | API-2 |
|
||||
| `apps/edge/internal/service/single_request_metrics_test.go` | API-2 |
|
||||
| `apps/edge/internal/bootstrap/runtime.go` | API-2 |
|
||||
| `apps/edge/internal/bootstrap/single_request_observation_test.go` | API-2 |
|
||||
| `apps/edge/internal/openai/single_request_metrics.go` | API-2 |
|
||||
| `apps/node/internal/workspace/observation.go` | API-2 |
|
||||
| `apps/node/internal/workspace/runtime.go` | API-2 |
|
||||
| `apps/node/internal/workspace/cleanup.go` | API-2 |
|
||||
| `apps/node/internal/workspace/observation_test.go` | API-2 |
|
||||
| `apps/edge/internal/openai/single_request_handler_test.go` | API-3 |
|
||||
| `agent-spec/input/openai-compatible-surface.md` | API-3 |
|
||||
| `agent-spec/runtime/edge-node-execution.md` | API-3 |
|
||||
| `agent-task/m-iop-owned-single-request-agent-execution/14+05,12,13_workspace_observation/CODE_REVIEW-cloud-G08.md` | API-1, API-2, API-3 |
|
||||
|
||||
## Final Verification
|
||||
|
||||
1. `test -f agent-task/m-iop-owned-single-request-agent-execution/05+03_single_ingress/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/05+03_single_ingress/complete.log' | wc -l)" -eq 1`
|
||||
2. `test -f agent-task/m-iop-owned-single-request-agent-execution/12+05,08,11_internal_tool_loop/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/12+05,08,11_internal_tool_loop/complete.log' | wc -l)" -eq 1`
|
||||
3. `test -f agent-task/m-iop-owned-single-request-agent-execution/13+12_workspace_cleanup/complete.log || test "$(compgen -G 'agent-task/archive/*/*/m-iop-owned-single-request-agent-execution/13+12_workspace_cleanup/complete.log' | wc -l)" -eq 1`
|
||||
4. `go test -race ./apps/edge/internal/service ./apps/edge/internal/bootstrap ./apps/node/internal/workspace -run 'Test(SingleRequestObservation|SingleRequestMetrics|SingleRequestObservationWiring|WorkspaceObservation)' -count=1`
|
||||
5. `go test ./apps/edge/internal/openai -run 'TestAnthropicSingleRequestObservation' -count=1`
|
||||
6. `go test ./apps/edge/internal/service ./apps/edge/internal/bootstrap ./apps/edge/internal/openai ./apps/node/internal/workspace -count=1`
|
||||
7. `go vet ./apps/edge/internal/service ./apps/edge/internal/bootstrap ./apps/edge/internal/openai ./apps/node/internal/workspace`
|
||||
8. `rg --sort path -n 'stage.*pure|tool.*duration|cleanup|total|raw|cardinality|Claude.*defer' agent-spec/input/openai-compatible-surface.md agent-spec/runtime/edge-node-execution.md`
|
||||
9. `git diff --check`
|
||||
|
||||
Expected: all predecessors are uniquely complete; timing math, cardinality, privacy, failure isolation, and ingress-to-terminal deltas pass; docs defer external smoke. Cached tests are not acceptable.
|
||||
|
||||
**After completing all code changes, fill implementation-owned sections in `CODE_REVIEW-*-G??.md`.**
|
||||
Loading…
Reference in a new issue