feat(agent-config): three-state field access over reporting instances [4/21] - #325
gusfcarvalho wants to merge 3 commits into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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 |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Source reuse, plugin re-enabling, and registry parsing currently diverge from API classification behavior.
Review effort: Balanced
Findings: 3
Open (3)
What changed in this PR
Adds agent configuration field-access classification across reporting instances.
Changes:
- Computes editable, restricted, read-only, and forbidden states.
- Classifies plugin sources and installation eligibility.
- Adds comprehensive unit tests for access rules and tooltips.
| File | Description |
|---|---|
src/utils/agent-config/field-access.ts |
Implements field access and source classification. |
src/utils/agent-config/__tests__/field-access.spec.ts |
Tests field states, source safety, and plugin installation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| case 'source': | ||
| return rc.trusted.length ? YES : no(NO_TRUSTED); |
There was a problem hiding this comment.
Fixed in b25fd2f. On an apply_safe instance without trusted_sources, source is now partial ("only a source this host already uses") when the host file uses any source, because the API classifies that as already-used. It stays read-only only when there is nothing to reuse. usedSources also skips disabled plugins now, matching the API.
| default: | ||
| return YES; |
There was a problem hiding this comment.
Fixed in b25fd2f. When the loaded base disables the plugin, enabled follows reenables-plugin: it applies only when the effective source (the overlay source, else the file source) matches trusted_sources, the kept policy entries classify as safe, and the effective config has no ${env:} references (TestClassifyReenableKeptParts). Otherwise it is no with a re-enable reason. Disabling or a no-op stays data-only. Without a loaded base it stays advisory.
e1d9d65 to
44b07ac
Compare
| return rc.trusted.length | ||
| ? YES | ||
| : { verdict: 'partial', reason: NO_TRUSTED_POLICIES }; | ||
| default: |
There was a problem hiding this comment.
[Must-fix] Re-enabling a disabled plugin is shown as editable under apply_safe, but the API rejects it
Introduced in #325. Under apply_safe, plugins.<p>.enabled falls through to default: return YES (the header comment lists enabled as Safe). The API's Classify (pkg/agentconfig/classify.go, reenables-plugin) marks turning a plugin the host disabled back on as Unsafe unless its source matches trusted_sources. With an apply_safe host, empty trusted_sources and a base plugin {enabled:false}, the UI shows a plain pencil on enabled; saving produces will-apply:false, unsafe-changes on every instance.
Why: The field-access prediction is the UI's whole promise about what will apply; here it says 'all would apply' for a change no apply_safe instance applies.
Fix: When the base has the plugin disabled, give enabled the same verdict as source (YES only if the source is trusted; otherwise no(...) with a re-enable reason), and add a field-access spec for it.
Severity basis: no rule matched; decision tree: wrong behaviour or missing companion change.
ccf-review · 720b4a985b63 · rules@6be9e11b8bc6
There was a problem hiding this comment.
Fixed in b25fd2f. Under apply_safe, plugins.<p>.enabled on a plugin the host file disables now gets the re-enable verdict: trusted effective source (otherwise no: "re-enabling a plugin this host disabled needs a trusted source"), and, as in classify.go, its kept policies and ${env:} references are re-checked. Specs are in field-access.spec.ts, and the cases are also in the new conformance table.
| @@ -0,0 +1,488 @@ | |||
| // Field states on the Effective view (R71), derived per instance from the API's | |||
There was a problem hiding this comment.
[Should-fix] CORE-DUP-001 · Logic or literal duplicated where it must stay in sync
Introduced in #325 (with glob.ts and cron5.ts in #324 and NAME_RE in #326). field-access.ts re-implements pkg/agentconfig Classify/WillApply, glob.ts re-implements path.Match for trusted_sources, cron5.ts re-implements the robfig parser and validation.ts copies PluginNamePattern. They have already drifted: re-enabling a plugin (field-access.ts:185) and time zone case (cron5.ts:153).
Why: Copies drift: one gets fixed or changed and the other does not.
Fix: Add a shared set of test cases (base config, overlay, remote_config, and the API's expected Classify/WillApply verdict), generated from api pkg/agentconfig's own tests and asserted by the UI's field-access, glob, cron5 and plugin-name tests, so any drift fails a test.
ccf-review · 14ae7be8ed98 · rules@6be9e11b8bc6
There was a problem hiding this comment.
Partly addressed in b25fd2f (and a5066a0 for plugin names). I am leaving this thread open for you to judge.
Done (UI side): src/utils/agent-config/__tests__/fixtures/agentconfig-conformance.json is one table of expected results taken from the API's own pkg/agentconfig tests at api@83ed7d6:
- TestMatchTrustedSource and TestMatchOverridableConfigFlag, for
glob.ts - TestKindOfAndIsOCISource, for
field-access.tssourceKind - TestNamePatterns, for
NAME_RE - TestParseScheduleTimeZonePrefix, for
cron5.ts - Classify→WillApply apply_safe cases, including TestClassifyReenableKeptParts, for
fieldAccess
agentconfig-conformance.spec.ts asserts all of them, so a rule that changes on one side now fails a test once the table is updated. Writing it surfaced and fixed two more drifts besides the two you named:
- the UI's
usedSourcescounted disabled plugins, which the API does not %was accepted in a registry authority (Copilot's thread)
The cron time zone case is fixed in #324 (6cc9c63): zone names are now case-sensitive, as in Go.
Not done: generating the table from the API. Today it is transcribed, and its _source field names the API commit. The full fix is an API-side change: have the agentconfig tests write (or check against) this golden file, plus a sync step or CI check in the UI. That spans both repos, so I would rather do it as a follow-up than grow this stack. Happy to do it here if you prefer.
44b07ac to
b25fd2f
Compare
gusfcarvalho
added
the
size-exception
Layer 4 of 21 in the stacked split of #318. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sources
- apply_safe: re-enabling a plugin the host's file disables applies only
with a trusted effective source, and re-checks the policies and ${env:}
references it keeps (classify.go reenables-plugin)
- apply_safe without trusted_sources: reusing a source the file already
uses is Safe, so `source` is partial instead of read-only
- usedSources skips disabled plugins, as the API does
- registry authorities with % are local sources (Go url.Parse)
- agentconfig-conformance.json: one table of expected results from the
API's pkg/agentconfig tests, asserted against glob, cron5 and
field-access so the copies cannot drift silently
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
FORBIDDEN_TOOLTIP reads TOOLTIPS['agents.config.field.forbidden'], per docs/TOOLTIPS.md (UI-COMP-001). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
b25fd2f to
93dbcf6
Compare
Part 4 of 21 of the stacked split of #318 (agent remote configuration). Every layer adds the final version of its files from #318, and only imports from layers below it, so each layer passes
make reviewableon its own. Nothing is reachable in the app until layer 20 wires the Configuration tab in.What
Computes three-state field access (editable / locked / unsupported) for each config path across the instances that reported, from the API's locked-key globs and per-instance capabilities.
Tests
field-access.spec.ts.🤖 Generated with Claude Code