POC: prepare and apply desktop standup plans through MCP - #331
Conversation
|
Please review head 8508d77. Focus on MCP argument confinement, draft/confirmation version binding, preparation having no Fleet effects, reuse of the apply compiler, and truthful status under missing or stale evidence. This POC trusts the desktop agent to relay user words; it does not claim audio authentication. Report actionable findings against this exact head; do not edit code. |
|
Claude finished @itsHabib's task in 6m 46s —— View job Review — head
|
| # | Area | Severity | Finding |
|---|---|---|---|
| F1 | MCP confinement | P3 / doc | plan bytes skip values; defense is in DecodeDraft—a comment at mcp.go:111 would explain the delegation |
| F2 | MCP confinement | P3 / test | TestMCPConversation checks Surface but not By; add assertion for Confirm.By == "human:acme" |
| F3 | Version binding | observation | Stale-digest and execution-history checks are ordered correctly and serve independent invariants |
| F4 | No Fleet effects | P3 / doc | Prepare result is emitted before the error check; MCP callers see both data and isError: true simultaneously on planning refusal |
| F5 | Compiler reuse | P2 / CLI | markStatus(steps, done, true) ignores ledger-done status in prepare output; unreachable via MCP but misleading on CLI |
| F6 | Truthful status | P3 | observe treats JSON null as "unreadable Fleet output"; conservative but misleading message |
| F7 | Truthful status | P2 / test | Delivery observation uses tenantRequired=false; cross-tenant workers sharing address+cwd would appear in warnings |
No correctness, security, or authorization invariant failures found at this head. The five focus areas are sound: arguments are confined, version binding is correct at both the draft and apply boundaries, prepare calls no write verb, both paths share prepareSteps, and status derives state from evidence rather than process activity.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8508d774af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| seats, err := e.Seats(cfg.Tenant) | ||
| if err != nil { | ||
| return s, err |
There was a problem hiding this comment.
When roles.map is missing or unreadable, this return causes standup_status to emit only an error, discarding the collected runtime observation and all per-card unknown results. This also breaks status for empty or checkout-only plans that do not need a seat lookup. Keep the structured status response and record the lookup failure as unknown evidence instead, as required for missing source data.
AGENTS.md reference: cmd/standup/AGENTS.md:L83-L84
Useful? React with 👍 / 👎.
|
Fix round 1, head e117219. Consolidated initial findings:
Focused race tests, vet, lint, and the updated standup suite passed. Please re-review this exact head without editing code. No merge requested. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate issues affect read-only guarantees, tenant isolation, and confirmation security.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a local MCP interface for creating, editing, preparing, confirming, applying, and monitoring persisted standup plans through Fleet.
Changes:
- Adds digest-guarded draft editing and MCP tools.
- Adds preparation, status, receipt reporting, and store locking.
- Adds tests and desktop voice-trial documentation.
File summaries
| File | Description |
|---|---|
cmd/standup/README.md |
MCP usage and desktop trial documentation |
cmd/standup/planning.go |
Planning handlers and locking |
cmd/standup/planning_test.go |
Planning and MCP tests |
cmd/standup/mcp.go |
JSON-RPC MCP adapter |
cmd/standup/mcp_schema.go |
MCP tool schemas |
cmd/standup/main.go |
CLI integration and digest checks |
cmd/standup/main_test.go |
Runtime and receipt fixtures |
cmd/standup/internal/standup/prepare.go |
Preparation and receipt observations |
cmd/standup/internal/standup/draft.go |
Draft decoding and updates |
cmd/standup/internal/standup/apply.go |
Planning and step serialization |
cmd/standup/CLAUDE.md |
MCP implementation guidance |
cmd/standup/AGENTS.md |
MCP implementation guidance |
Review details
Suppressed comments (4)
cmd/standup/main.go:67
- The lock acquired here is always
$STANDUP_DIR/.lock, but these mutating handlers still accept path references (andnew --outcan target an arbitrary path). Two callers can therefore update the same out-of-store record under different locks; atomic rename does not prevent a lost update. Either restrict locked operations to store IDs or derive the lock from the resolved target path.
release, err := lockStore(env)
if err != nil {
return fail(stderr, err)
}
defer release()
cmd/standup/main.go:240
- The digest check does not establish record identity. Because
cfgis already loaded, a valid record for another tenant/lead in the same store can be confirmed and persisted through this path; apply will reject it only later. Rejectr.Tenant/r.Leadmismatches beforeConfirmRecord.
if err := expectDigest(r, *expected); err != nil {
return fail(stderr, err)
}
cmd/standup/mcp.go:64
- All errors from
rpcResultare mapped to-32602, so an unknown top-level JSON-RPC method is reported as Invalid Params. MCP/JSON-RPC clients rely on-32601to distinguish an unsupported method from malformed parameters; map the unknown-method case separately, as the other MCP servers in this repository do.
result, err := rpcResult(req)
if err != nil {
return rpcError(req.ID, -32602, err.Error())
}
cmd/standup/planning.go:82
- This new MCP-reachable mutation loads any record ID in the configured store and passes it to
UpdateDraftwithout checkingr.Tenant/r.Leadagainst the configured identity. A foreign record retained in the store can therefore be rewritten (and its confirmation cleared) from this tenant's server, even though apply later refuses it. Load the config and reject an identity mismatch before updating the draft.
path := env.ResolveRecord(fs.Arg(0))
r, err := standup.LoadRecord(path)
if err != nil {
return fail(errOut, err)
}
if err := standup.UpdateDraft(r, d, *expected); err != nil {
return fail(errOut, err)
- Files reviewed: 12/12 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if moved != "" && !forceStale { | ||
| return nil, nil, "", refuse("the world moved since agenda %s:\n%s\nre-run standup agenda and re-plan, or apply --force-stale to proceed anyway", agenda.ID, moved) | ||
| } | ||
| world, err := ReadWorld(e, cfg) |
| s.Receipt.Error = err.Error() | ||
| return s | ||
| } | ||
| s.Receipt = observe(e, dir, "done", c.Change, "--kind", c.As, "--json") |
| func printView(out, errOut io.Writer, path string, r *standup.Record) int { | ||
| return printJSON(out, errOut, map[string]any{"path": path, "plan_digest": r.PlanDigest(), "record": r, "readback": r.Readback()}) |
|
Claude finished @itsHabib's task in 3m 44s —— View job Review — Fix Round 1, head
|
| Finding | Status |
|---|---|
| Codex: missing-role-map status | ✓ Fixed and tested |
| F5: accurate step status in prepare | ✓ Fixed and tested |
| F7: explicit cross-tenant delivery rejection | ✓ Fixed and tested |
| F1: plan bytes delegation comment | ✓ Documented |
| F2: Confirm.By assertion | ✓ Tested |
| F4: mcpInstructions isError note | ✓ Documented |
| F6: Fleet null output message | ✓ Clarified |
| New: agenda source warnings in Prepare | ✓ Added and tested |
| Copilot: DecodeDraft null acceptance | Not addressed (Copilot P2) |
| Copilot: printView phrase exposure | Not addressed (POC design decision) |
| Copilot: Fleet CLI migration in read paths | Not addressed (structural/POC scope) |
No correctness, security, or authorization invariant failures found at this head. The five focus areas are sound at e117219.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e117219aed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| keep := []any{} | ||
| for _, v := range workers { | ||
| row, ok := v.(map[string]any) |
There was a problem hiding this comment.
When roles.map and Fleet identify the same checkout through different path spellings—such as a symlink versus its resolved path, /var versus /private/var, or case differences on Windows—this raw string comparison drops the matching worker and reports that no runtime target was observed. Fleet's own identity matching canonicalizes both paths, so valid, deliverable seats can otherwise be presented by prepare as unavailable; canonicalize both values before comparing them.
Useful? React with 👍 / 👎.
|
Final fix round is now at Verified and fixed together: Fleet work/receipts/decisions/done observations could run legacy migration; draft/confirm could mutate a record for another configured tenant or lead; CLI JSON null could clear editable plan fields. Existing observation verbs now avoid migration and refuse actual legacy ambiguity, and the shared identity check precedes draft/confirmation. Fresh observation requires no migration marker. Added CLI/MCP identity and checkpoint-preservation tests and checked a compiled Fleet binary against fresh/legacy private stores. Passed: scoped Standup/Fleet tests, vet, lint, focused race tests; compiled-binary observations made no Fleet state changes. Independent exact-head verification is underway. No live confirmation, dispatch, installation or voice trial was performed by this fix. Consolidated residuals: external CLI record paths used from distinct STANDUP_DIR roots lack cross-store serialization; unknown JSON-RPC method error code remains a small protocol issue. MCP restricts references to store IDs. Confirmation phrase exposure does not create audio authentication: the documented POC trusts the desktop agent to relay actual words. Explicit cross-tenant delivery filtering was already addressed at e117219 and was not changed again. This is fix round two. The final panel is being requested once; no fourth cycle. Verified final P1/authorization findings will be addressed with evidence, and lower-priority residuals will retain written disposition. Trial/merge holds remain in force. |
|
@codex review |
|
@claude review |
|
Claude finished @itsHabib's task in 2m 40s —— View job Review — Fix Round 2, head
|
| Test | Covers |
|---|---|
TestObservationalDispatchNeverMigrates |
Fresh-state no-op, legacy refusal, state immutability, ReadOnly restore |
TestForeignRecordMutationRefused |
tenant and lead mismatch, CLI and MCP, record immutability |
TestNullDraftPreservesRecord |
JSON null rejection, record immutability |
All three new tests are assertion-specific and not trivially satisfiable.
5. Residuals (author-documented)
| Item | Status |
|---|---|
External --out paths lack cross-store locking |
Documented; lockStore uses $STANDUP_DIR/.lock always; new --out can target outside the store |
Unknown JSON-RPC method maps to -32602 not -32601 |
Documented; low impact; protocol issue only |
printView exposes Confirm.Phrase |
Documented POC trust boundary; no change |
Checkout path canonicalization in scopeWorkers (Codex P2) |
Documented; unaddressed; delivery warnings only |
These are correctly carried forward per the review-cycle discipline: verified-addressed P1s at this head, lower-priority residuals with written disposition, no fourth panel round.
Summary
| Finding | Status at bddbcc6 |
|---|---|
| Fleet legacy migration in observational paths | ✓ Fixed and tested |
| draft/confirm identity mismatch across tenants/leads | ✓ Fixed and tested |
| JSON null clears editable plan fields | ✓ Fixed and tested |
| Residuals (cross-store lock, RPC error code, phrase exposure, path canon) | Documented; within POC scope |
No correctness, security, or authorization invariant failures found at bddbcc6. The five original focus areas remain sound; the three final-round P1 fixes are verified correct.
|
Reviewer: claude |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Final disposition at Residuals retained: external CLI record paths across distinct stores lack shared serialization; unknown JSON-RPC method error code; path canonicalization can cause false unavailable-seat warnings; phrase disclosure remains within the explicitly trusted-agent POC model and is not audio authentication. The The two-fix-round limit is reached. No further panel loop or Gate/merge attempt. Draft/operator-trial boundary remains intact. Workbench #332 has independently merged; reconcile its small Dispatch insertion when this POC is next prepared to land. |
|
Integrated merged main (#332) at |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved correctness and safety issues remain in locking, confirmation handling, input validation, and Fleet observation paths.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (8)
Previously missed (1) — in code that hasn't changed since the last review.
cmd/standup/internal/standup/prepare.go:102
- These comparisons use raw strings, but Fleet's status projections canonicalize paths, including symlinks and platform aliases, before matching. If
roles.mapcontains an alias, a valid worker is emitted with the canonicalcwdand is dropped here, so prepare/status incorrectly report no runtime target. Use the same canonical directory comparison forwantedand the rowcwdwhile preserving the raw path for execution.
cmd/fleet/internal/fleet/lease.go:440
- This guard is documented as catching unreadable ownership records, but
unresolvedLegacyrelies onlistDir, which returns nil for both a missing and an unreadable/non-directory path. Ifleases,stop, orhandoffcannot be read,LegacyKeysPresentreturns false and observational commands proceed with an incomplete state view instead of refusing. Preserve directory read errors and treat non-ENOENT failures as unresolved.
for _, sub := range []string{"leases", "stop", "handoff"} {
if unresolvedLegacy(sub) {
return true
cmd/standup/internal/standup/draft.go:24
- The MCP schema marks
roles,cards,decisions, anddeferredas required, but decoding into zero-value slices accepts a plan such as{}.UpdateDraftthen replaces every editable field with empty slices, so a caller/model that omits one field can erase the existing plan while supplying a valid digest. Enforce presence of each non-optional field at the decode seam, or explicitly change the schema and implement merge semantics.
dec := json.NewDecoder(io.LimitReader(in, 1<<20))
dec.DisallowUnknownFields()
if err := dec.Decode(&d); err != nil {
cmd/standup/internal/standup/draft.go:23
- Because the decoder is fed an
io.LimitReadercapped at exactly 1 MiB, an object whose closing byte lands at the limit is followed by EOF from the limited reader; any extra JSON or garbage in the original input is silently ignored. That bypasses the claimed exact-one-object/trailing-input protection at the boundary. Track overflow (for example, read one extra byte or use a counting reader) and reject inputs larger than the limit.
dec := json.NewDecoder(io.LimitReader(in, 1<<20))
dec.DisallowUnknownFields()
cmd/standup/internal/standup/prepare.go:80
- This runtime snapshot uses
fleet status --all, but that top-level command goes directly towatch.AllStatusand bypasses the legacy-state guard added forwork/receipts/decisions/done. With a legacy or unreadable lease,AllStatussilently drops it fromleaseRowsand can return a normal snapshot showing a vacant/unoccupied binding, so standup status reports incorrect evidence instead of unknown/refused. Add a legacy preflight to thestatus --all/AllStatuspath before relying on this observation.
o := observe(e, e.LeadDir, "status", "--all", "--json")
scopeWorkers(e, r, &o, true)
cmd/standup/main.go:224
--surfaceis documented and MCP-schema-restricted totext|voice, but the shared CLI path forwards any other nonempty value toConfirmRecord, which stores it and reports it in readback. A directstandup confirm --surface=...can therefore create records outside the advertised contract and misstate attribution; validate the enum in the shared confirmation/record validation, not only in MCP.
surface := fs.String("surface", "text", "text or voice")
cmd/standup/main.go:254
--expectis now a supported apply flag, but the top-level usage and the parse-error usage still omit it. Users followingstandup --helpwill not discover the digest guard that the README says CLI apply accepts; update both usage strings.
expected := fs.String("expect", "", "plan digest from readback")
cmd/standup/planning.go:54
- This MCP-facing view serializes
*Recordverbatim, so a confirmed record exposesrecord.confirm.phrase. After one legitimate confirmation, the desktop agent can callstandup_show, reuse that revealed phrase on a new record, and passstandup_confirmwithout the user saying it, defeating the documented actual-words gate. Return a redacted MCP record (or omitConfirm.Phrase) while retaining the phrase only for verification.
return printJSON(out, errOut, map[string]any{"path": path, "plan_digest": r.PlanDigest(), "record": r, "readback": r.Readback()})
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Lite
| func lockStore(env standup.Env) (func(), error) { | ||
| if err := os.MkdirAll(env.Dir, 0o755); err != nil { | ||
| return nil, err | ||
| } | ||
| f, err := os.OpenFile(filepath.Join(env.Dir, ".lock"), os.O_CREATE|os.O_RDWR, 0o600) |
Late final-review disposition — c0c622cReconciled Copilot's late review of bddbcc6 against the combined current head. Read-only source inspection and isolated overlay/subprocess probes confirmed the defects below. No new demonstrated P1 or authorization bypass was found. The two-fix-round cap is exhausted, so no additional fixes or panel cycle were opened. Existing draft, operator voice-trial, and no-Gate/no-merge boundaries remain intact. Verified P2 residuals:
P3: usage text omits apply's --expect flag. Phrase disclosure does not constitute an authentication bypass in this POC's explicit trusted-agent model. Redacting the phrase would not authenticate audio or establish that the user spoke it; no new authentication scheme is justified by this finding. Before a voice trial, surface these residuals in its preparation: send complete explicit draft arrays, inspect saved changes before readback, keep records inside one configured store, and do not interpret a normal occupancy response as complete evidence when ownership storage is malformed or legacy. These are trial constraints, not claims the defects are fixed. The installed POC has not been refreshed or validated by this reconciliation, and no live confirmation or dispatch occurred. |
Summary
A desktop voice or text conversation can now build a persisted standup plan through a local MCP connection, check what it would do before confirmation, then apply that exact version through the existing Fleet compiler. Preparation no longer requires manufacturing a confirmation just to reach a dry run.
What this adds
standup mcp: agenda, new, draft, show, prepare, confirm, apply and status, using the same CLI handlers and store. No custom audio transport or new launcher.cmd/standup/README.md.Validation
go vet ./...andgolangci-lint run ./...passed.go test ./...passed; focused standup race tests passed.POC only. No production delivery configuration, worker launch, or merge is part of this change.