docs: agent-config-editing design workspace (stack bottom) - #5733
Conversation
Planning docs (context, plan, status, research index) plus the accepted research inputs: the RFC, the change-set interface spec, and the runner lifecycle architecture.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds a complete design workspace for agent configuration editing. It documents editing contracts, revision transactions, reads, workspace imports, authorization, runner lifecycle reconciliation, model-usability spikes, design reviews, rollout planning, and project status. ChangesAgent configuration editing
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Both spikes are green. The decisions log records what was accepted from each spike and the seven product calls still open.
change-set, commit-transaction, read-config (engine-spike); execution-authorization, workspace-import, adapter-matrix (runner-spike). The value_from schema conflict is arbitrated; twelve product calls open.
…lls deduped to six
…orded Strict canonical serializer, multi-source atomicity, honest TOCTOU model, descendant confinement, harmless-forgery acknowledgement design, canonical generation payload, text-file import into text fields with diff approval, four-layer executable permission split.
…witch, S7c0 foundation slice, accepted Daytona risk
…ions, six open calls
…es, refactor plan, decision context
…ctions; inline marker validated; harness included
… errors, path normalization, selector forgiveness, optional free-text on all tools
…ditions; message derived server-side; invalid_operation split; v3 spike results
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (2)
docs/design/agent-config-editing/spikes/model-usability/harness.py (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the two unused imports. Flake8 reports F401 for both spike scripts; neither module is referenced.
docs/design/agent-config-editing/spikes/model-usability/harness.py#L16: deleteimport os.docs/design/agent-config-editing/spikes/model-usability/run.py#L12: deleteimport copy.Source: Linters/SAST tools
docs/design/agent-config-editing/spikes/model-usability/run.py (1)
379-413: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueClose the output file with a context manager.
If
pool.mapraises, line 413 is never reached and the buffered records stay unflushed. Wrap the pool run in awith out.open("w") as handle:block so partial results survive a failed run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b018c86-a927-402b-8092-90786eaf6c7d
⛔ Files ignored due to path filters (1)
docs/design/agent-config-editing/spikes/model-usability/results.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (38)
docs/design/agent-config-editing/BRIEFING.mddocs/design/agent-config-editing/README.mddocs/design/agent-config-editing/context.mddocs/design/agent-config-editing/contracts/adapter-matrix.mddocs/design/agent-config-editing/contracts/change-set.mddocs/design/agent-config-editing/contracts/commit-transaction.mddocs/design/agent-config-editing/contracts/execution-authorization.mddocs/design/agent-config-editing/contracts/read-config.mddocs/design/agent-config-editing/contracts/workspace-import.mddocs/design/agent-config-editing/decisions.mddocs/design/agent-config-editing/notes/dao-lock-impact.mddocs/design/agent-config-editing/notes/final-review-findings.mddocs/design/agent-config-editing/notes/s7e-credential-handoff.mddocs/design/agent-config-editing/open-issues.mddocs/design/agent-config-editing/plan.mddocs/design/agent-config-editing/research.mddocs/design/agent-config-editing/research/change-set-interface-codex.mddocs/design/agent-config-editing/research/design-gate-review-codex.mddocs/design/agent-config-editing/research/design-gate2-review-codex.mddocs/design/agent-config-editing/research/design-gate3-review-codex.mddocs/design/agent-config-editing/research/rfc.htmldocs/design/agent-config-editing/research/runner-lifecycle-codex.mddocs/design/agent-config-editing/spikes/engine-spike.mddocs/design/agent-config-editing/spikes/model-usability-spike.mddocs/design/agent-config-editing/spikes/model-usability/analyze.pydocs/design/agent-config-editing/spikes/model-usability/harness.pydocs/design/agent-config-editing/spikes/model-usability/instructions/v0.mddocs/design/agent-config-editing/spikes/model-usability/instructions/v1.mddocs/design/agent-config-editing/spikes/model-usability/instructions/v2.mddocs/design/agent-config-editing/spikes/model-usability/instructions/v3.mddocs/design/agent-config-editing/spikes/model-usability/instructions/v4a.mddocs/design/agent-config-editing/spikes/model-usability/instructions/v4b.mddocs/design/agent-config-editing/spikes/model-usability/run.pydocs/design/agent-config-editing/spikes/model-usability/selftest.pydocs/design/agent-config-editing/spikes/model-usability/table.pydocs/design/agent-config-editing/spikes/model-usability/tasks.pydocs/design/agent-config-editing/spikes/runner-spike.mddocs/design/agent-config-editing/status.md
| - **There is no folder source and no folder-to-skill codec in v1.** The agent authors the | ||
| skill structure itself and references each file's content per field. It already knows the | ||
| structure; it does not need a codec to infer it. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Resolve the US-3 folder-import regression.
US-3 promises that an agent imports a downloaded skill by naming its folder. The decided change-set removes the folder codec and requires one marker per file. The authorization contract then limits a call to eight markers. A downloaded skill with more than eight referenced text files cannot complete in one atomic import.
docs/design/agent-config-editing/contracts/change-set.md#L488-L490: restore a folder representation, or record an explicit product decision that changes US-3.docs/design/agent-config-editing/BRIEFING.md#L19-L22: align the stated user story with the selected import model.docs/design/agent-config-editing/context.md#L33-L36: align the stated goal with the selected import model.docs/design/agent-config-editing/contracts/execution-authorization.md#L435-L443: if marker imports remain, set limits that permit a complete skill import or define a safe multi-commit workflow.
📍 Affects 4 files
docs/design/agent-config-editing/contracts/change-set.md#L488-L490(this comment)docs/design/agent-config-editing/BRIEFING.md#L19-L22docs/design/agent-config-editing/context.md#L33-L36docs/design/agent-config-editing/contracts/execution-authorization.md#L435-L443
There was a problem hiding this comment.
Acknowledged, fix queued. BRIEFING.md and context.md still state US-3 as "point at its folder," but the decided change-set removed the folder codec (change-set.md 6.2) and execution-authorization.md 6.2 caps a call at 8 sources / a turn at 32. A downloaded skill with more referenced text files than that cannot land in one atomic import. This needs a product decision (raise the limit, or restate US-3 as a multi-commit workflow), not a doc tweak, so leaving open for that decision rather than resolving.
There was a problem hiding this comment.
@mmabrouk, agreed. The finding remains open.
The selected marker model cannot guarantee a one-commit import for every folder-based US-3 skill under the current 8-source limit. The product decision must either:
- Increase the authorization limit to support the defined skill size, or
- Change US-3 to define a safe multi-commit import workflow.
After that decision, the contracts and user-story documents can align.
🐇
You are interacting with an AI system.
| The runner then substitutes the frozen value into the call body. It replaces `value_from` with | ||
| `value`. It never rereads the folder. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Substitute the marker at valuePointer.
@ag.file is nested inside an operation value. It is not an operation-level value_from field. Replace the marker at each authorized {operationIndex, valuePointer} with its frozen string on a copied call body.
Do not describe this step as replacing value_from with value. That obsolete transformation can leave markers unresolved or construct an invalid operation body.
Also applies to: 334-335
There was a problem hiding this comment.
Acknowledged, fix queued. Section 3.3 ("It replaces value_from with value") and 3.4.4 ("The runner replaces value_from with value for every required index") still use the pre-consolidation phrasing, even though 3.4's own banner says the source moved from an operation-level value_from field to an inline @ag.file marker anywhere in value. Needs a wording pass to describe substitution at the marker's valuePointer, not value_from replacement, so leaving open.
…rding, open issues)
dosubot
Bot
added
size:XXL
The spike harness could not run: it imported its engine from a worktree that no longer exists. Its hand-written schema had also drifted from the shipped one, and the field whose placement a live model got wrong did not exist in it at all. real_surface.py imports both surfaces instead of copying them: the engine from api/oss/src/core/workflows/change_set.py and the commit schema from op_catalog.py with ordered operations on. It also reports the divergences, so a run states what changed rather than assuming. Adds two scored envelope tasks, a schema check at the point a harness would make it, and 371 trials across four models in two arms. The result: the placement sentence has no measured effect, because three of four models never make the mistake and the fourth is not helped by it. selftest.py used the pre-v3 'field' selector key and refused every golden delta; it passes against the shipped engine now.
…oposal, three shippable layers) and the denied-sibling seam; AGENTS.md gains today's GitButler dropped-hunk, attribution, and cliId lessons
… is whether the running harness OBSERVED the change); open-issues gains the refresh-then-reopen follow-up with its two prerequisites
…against rev-parse only
…tack; ask which lane tip first contains every symbol it touches (three in-isolation CI reds in one day before this rule)
… 750ms poll for an out-of-order frame; a stack slow enough to miss it degrades to a gate that mints nothing)
docs: agent-config-editing design workspace (stack bottom)
1 participant
Context
This is the bottom PR of the agent-config-editing stack. The feature: a playground agent can read and edit its own stored configuration through targeted operations, with human approval gating anything that imports content from its workspace. This PR carries no product code. It holds the design workspace the other seventeen PRs implement, so reviewers can read intent before diffs, and inline-comment on decisions rather than on code.
What is in here
BRIEFING.md: the reviewer document. Start here. Written in simple technical English; every decision carries its context.decisions.md: every product and engineering ruling with its rationale, including the rotation ruling and the scope-enforcement seam.contracts/: the six behavior contracts (change-set, commit-transaction, read-config, workspace-import, execution-authorization, adapter-matrix), synced to shipped behavior after the final review round.plan.md,status.md,open-issues.md: execution plan, final state, and the recorded follow-ups.spikes/: the three spike reports (engine, runner, model usability) with their raw data and the reusable instruction-testing harness.notes/: the dao-lock impact analysis and the final external review findings, verbatim.Reading order for the stack
Bottom up: s5, s4 (runner safety, description field), s1a, s6, s1b-lock, s1b (engine, coordinator, lock, commit wrapper), s7a, s2 (lifecycle units, read_config and commit hardening), s7b, s3a, s7c (context acquisition, imports, facets), s3b-core, s7d, s7e (authorization core, reopen, credentials), then the three wiring PRs (runner, SDK, web). Each PR's base is the lane below it, so each shows only its own diff.
Notes
AGENTA_WORKFLOWS_ORDERED_OPERATIONS_ENABLED; the flag off keeps today's behavior and the legacy delta form keeps working in both states.status.md; findings verbatim innotes/final-review-findings.md.