Skip to content

fix(cli): make af config write through to the secret store + pre-install requirements visibility#819

Merged
AbirAbbas merged 10 commits into
mainfrom
fix/config-secrets-coherence
Jul 24, 2026
Merged

fix(cli): make af config write through to the secret store + pre-install requirements visibility#819
AbirAbbas merged 10 commits into
mainfrom
fix/config-secrets-coherence

Conversation

@AbirAbbas

Copy link
Copy Markdown
Contributor

Problem

After af install, the post-install guidance for missing required env vars told users to run af config <node> --set KEY=value. But af config --set wrote only the package .env (cli/config.go SetVariablesaveEnvFile), and both af run code paths ignore that file — they resolve a node's environment from the encrypted secret store via packages.EnvResolver (packages/env_resolver.go), not the .env. So the value looked saved but never reached the running process. Meanwhile af config printed "Run 'af run …' to start the agent with these settings", which was false. The .env is read by af dev (core/services/dev_service.go) and the web UI env editor (handlers/ui/env.go), so it can't simply be dropped.

Two related gaps:

  • When a required secret was missing and stdin was not a TTY, startup hard-failed with missingEnvError (packages/env_resolver.go) listing variable names but no fix command and no node name.
  • There was no way to see what env vars a node needs before installing it; the docs (docs/installing-agent-nodes.md) only ever taught af secrets, never af config.

What changed

  1. af config <node> --set K=V now writes through to the node-scoped encrypted secret store (the same store af secrets set K --node <node> uses) in addition to the .env (kept for af dev + the web UI). Interactive af config and --unset mirror to the store too, so the .env and the runtime environment can no longer drift. Output now states both destinations and the misleading copy is corrected.
  2. missingEnvError is actionable: it names the node and pairs every unset variable — and every option of an unsatisfied require_one_of group — with the exact af secrets set <VAR> --node <name> command.
  3. New af show-requirements <path-or-git-url>: resolves a source without installing (local path parsed in place; Git URL shallow-cloned to a temp dir that is removed afterwards — nothing is written under ~/.agentfield), then prints the node's required vars, optional vars with defaults, and require_one_of groups. Supports -o json. Mentioned in af install --help.
  4. Post-install output now prints explicit next steps — af run <name>, af list, and (when required vars exist) the correct af secrets set <VAR> --node <name> command.
  5. Docs: docs/installing-agent-nodes.md documents af show-requirements and the now write-through af config, consistent with the existing af secrets guidance.

Validation Contract → tests

  • After af config <node> --set K=V, the node-scoped secret store contains K and the .env also contains itTestSetVariableWritesThroughToSecretStore (cli/config_additional_test.go), which also asserts --unset removes from both. Interactive write-through covered in TestPackageConfigManagerInteractiveAndCommand.
  • Missing-secret startup failure contains af secrets set <VAR> --node <name> for each missing var (and each group option)TestMissingEnvError_IncludesActionableSecretsCommand and TestResolve_MissingSecretErrorCarriesFixCommand (packages/env_resolver_group_test.go).
  • af show-requirements on a fixture manifest (required + optional-with-default + require_one_of) outputs all three categories in text and JSON, and creates nothing under ~/.agentfield/packagesTestShowRequirements_JSONReportsAllCategories, TestShowRequirements_TextListsCategoriesAndFixCommands (cli/show_requirements_test.go, fixture cli/testdata/show-requirements-node/), plus TestInspectSource_* (packages/inspect_test.go).
  • Post-install output includes af run <name> and, when required vars exist, the secrets commandTestInstallPackage_PostInstallNextStepsAndSecrets (packages/installer_output_test.go).

Test / gate results (run locally against Go 1.25.4, Node 22)

  • gofmt -l on changed files: clean
  • GOFLAGS=-buildvcs=false go build ./...: pass
  • go vet ./...: pass
  • go test ./internal/cli/... ./internal/packages/...: pass
  • Full go test ./...: green except one pre-existing, unrelated failure — internal/server TestStartAndStopCoverAdditionalBranches — confirmed failing identically on the base origin/main (this PR does not touch internal/server).
  • golangci-lint run --new-from-rev=origin/main: 0 issues on the diff.
  • npm ci && npm run build (control-plane/web/client): pass.

🤖 Generated with Claude Code

AbirAbbas and others added 5 commits July 22, 2026 15:00
`af config <node> --set K=V` (and interactive `af config <node>`) only ever
wrote the package `.env`, but `af run` resolves a node's environment from the
encrypted secret store, not that file — so configured values looked saved yet
never reached the running process, and `af config` even told users to run
`af run` with settings it would ignore.

Mirror every value set via `af config` into the node-scoped secret store (the
same one `af secrets set K --node <node>` uses), keeping the `.env` for `af dev`
and the web UI env editor. `--unset` now removes from both destinations so an
unset value cannot linger and get re-injected. Success/interactive copy now
states both destinations truthfully.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When a required secret is missing and stdin is not a TTY, `af run` hard-failed
with an error that listed variable names but no way to fix them. Pair each unset
variable (and every option of an unsatisfied require_one_of group) with the exact
command that resolves it — `af secrets set <VAR> --node <name>` — and name the
node in the message so the command is copy-pasteable.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was no way to see what environment variables a node needs until after
installing it. `af show-requirements <path-or-git-url>` resolves the source
WITHOUT installing — a local path is parsed in place; a Git URL (with optional
@ref and //subdir selector) is shallow-cloned into a temp directory that is
removed afterwards, so nothing is written under ~/.agentfield.

It prints the node name, required variables, optional variables with their
defaults, and require_one_of groups, pairing each required variable with the
exact `af secrets set <VAR> --node <name>` command. Supports `-o json`. The gap
is now also called out in `af install --help`.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…teps

The post-install output told users to run `af config <node> --set VAR=...` for
missing required variables — a command that only wrote the package .env, which
`af run` ignores. Point it at `af secrets set <VAR> --node <name>` (which `af run`
reads) for both missing required variables and unsatisfied require_one_of groups,
and always print explicit next steps (`af run <name>`, `af list`) so a first-time
user is never left guessing.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Document the new `af show-requirements` command and the now write-through
`af config` behavior in the agent-node installation guide, consistent with the
existing `af secrets` guidance: `af config --set/--unset` mirrors into the
node-scoped secret store `af run` reads (keeping the `.env` for `af dev` and the
web UI), and the non-interactive missing-secret error now includes the fix
command. Adds both commands to the lifecycle reference table.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AbirAbbas
AbirAbbas requested a review from a team as a code owner July 22, 2026 20:05
@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

📊 Coverage gate

Thresholds from .coverage-gate.toml: per-surface ≥ 84%, aggregate ≥ 85%, max per-surface regression ≤ 1.0 pp, max aggregate regression ≤ 0.50 pp.

Surface Current Baseline Δ
control-plane 86.90% 87.40% ↓ -0.50 pp 🟡
sdk-go 92.50% 92.00% ↑ +0.50 pp 🟢
sdk-python 93.82% 93.73% ↑ +0.09 pp 🟢
sdk-typescript 91.05% 90.42% ↑ +0.63 pp 🟢
web-ui 84.76% 84.79% ↓ -0.03 pp 🟡
aggregate 85.55% 85.75% ↓ -0.20 pp 🟡

✅ Gate passed

No surface regressed past the allowed threshold and the aggregate stayed above the floor.

@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

📐 Patch coverage gate

Threshold: 80% on lines this PR touches vs origin/main (from .coverage-gate.toml:thresholds.min_patch).

Surface Touched lines Patch coverage Status
control-plane 307 82.00%
sdk-go 0 ➖ no changes
sdk-python 0 ➖ no changes
sdk-typescript 0 ➖ no changes
web-ui 0 ➖ no changes

✅ Patch gate passed

Every surface whose lines were touched by this PR has patch coverage at or above the threshold.

@AbirAbbas AbirAbbas left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 PR-AF Review — Merge Blocked — Must-Fix Found

🚫 Merge blocked. 2 must-fix issues found by automated review.

Automated multi-agent code review · PR-AF built with AgentField

15 findings · 🚫 2 blocking · 💬 13 advisory · 🔴 0 critical · 🟠 15 important · 🔵 0 suggestions · ⚪ 0 nitpicks

PR Overview

Problem

After af install, the post-install guidance for missing required env vars told users to run af config <node> --set KEY=value. But af config --set wrote only the package .env (cli/config.go SetVariablesaveEnvFile), and both af run code paths ignore that file — they resolve a node's environment from the encrypted secret store via packages.EnvResolver (packages/env_resolver.go), not the .env. So the value looked saved but never reached the running process. Meanwhile af config printed "Run 'af run …' to start the agent with these settings", which was false. The .env is read by af dev (core/services/dev_service.go) and the web UI env editor (handlers/ui/env.go), so it can't simply be dropped.

Two related gaps:

  • When a required secret was missing and stdin was not a TTY, startup hard-failed with missingEnvError (packages/env_resolver.go) listing variable names but no fix command and no node name.
  • There was no way to see what env vars a node needs before installing it; the docs (docs/installing-agent-nodes.md) only ever taught af secrets, never af config.

What changed

  1. af config <node> --set K=V now writes through to the node-scoped encrypted secret store (the same store af secrets set K --node <node> uses) in addition to the .env (kept for af dev + the web UI). Interactive af config and --unset mirror to the store too, so the .env and the runtime environment can no longer drift. Output now states both destinations and the misleading copy is corrected.
  2. missingEnvError is actionable: it names the node and pairs every unset variable — and every option of an unsatisfied require_one_of group — with the exact af secrets set <VAR> --node <name> command.
  3. New af show-requirements <path-or-git-url>: resolves a source without installing (local path parsed in place; Git URL shallow-cloned to a temp dir that is removed afterwards — nothing is written under ~/.agentfield), then prints the node's required vars, optional vars with defaults, and require_one_of groups. Supports -o json. Mentioned in af install --help.
  4. Post-install output now prints explicit next steps — af run <name>, af list, and (when required vars exist) the correct af secrets set <VAR> --node <name> command.
  5. Docs: docs/installing-agent-nodes.md documents af show-requirements and the now write-through af config, consistent with the existing af secrets guidance.

Validation Contract → tests

  • After af config <node> --set K=V, the node-scoped secret store contains K and the .env also contains itTestSetVariableWritesThroughToSecretStore (cli/config_additional_test.go), which also asserts --unset removes from both. Interactive write-through covered in TestPackageConfigManagerInteractiveAndCommand.
  • Missing-secret startup failure contains af secrets set <VAR> --node <name> for each missing var (and each group option)TestMissingEnvError_IncludesActionableSecretsCommand and TestResolve_MissingSecretErrorCarriesFixCommand (packages/env_resolver_group_test.go).
  • af show-requirements on a fixture manifest (required + optional-with-default + require_one_of) outputs all three categories in text and JSON, and creates nothing under ~/.agentfield/packagesTestShowRequirements_JSONReportsAllCategories, TestShowRequirements_TextListsCategoriesAndFixCommands (cli/show_requirements_test.go, fixture cli/testdata/show-requirements-node/), plus TestInspectSource_* (packages/inspect_test.go).
  • Post-install output includes af run <name> and, when required vars exist, the secrets commandTestInstallPackage_PostInstallNextStepsAndSecrets (packages/installer_output_test.go).

Test / gate results (run locally against Go 1.25.4, Node 22)

  • gofmt -l on changed files: clean
  • GOFLAGS=-buildvcs=false go build ./...: pass
  • go vet ./...: pass
  • go test ./internal/cli/... ./internal/packages/...: pass
  • Full go test ./...: green except one pre-existing, unrelated failure — internal/server TestStartAndStopCoverAdditionalBranches — confirmed failing identically on the base origin/main (this PR does not touch internal/server).
  • golangci-lint run --new-from-rev=origin/main: 0 issues on the diff.
  • npm ci && npm run build (control-plane/web/client): pass.

🤖 Generated with Claude Code

Key Findings

2 issue(s) should be addressed before merge:

  • 🟠 .env writer escapes quotes that its consumers do not unescape (control-plane/internal/cli/config.go:344) — saveEnvFile changes values containing " into backslash-escaped text, but every named consumer only removes the surrounding quotes and never unescapes it. Gate: The .env writer escapes quotes but readers do not unescape, causing data corruption for values containing special characters.
  • 🟠 Git inspection allows a selected subdirectory to escape the clone through a symlink (control-plane/internal/packages/inspect.go:54) — A parsed //subdir selector is passed into findManifestDir, but the resolver’s containment check is lexical and then follows symlinks when checking the manifest. A Gate: Path traversal via symlink allows reading arbitrary files outside the intended clone root, posing an information disclosure risk.

13 advisory finding(s) surfaced as non-blocking:

  • 🟠 af config writes global declarations into the node scope (control-plane/internal/cli/config.go:348)
  • 🟠 af config does not support require_one_of options (control-plane/internal/cli/config.go:302)
  • 🟠 Missing-secret repair commands ignore declaration scope (control-plane/internal/packages/env_resolver.go:285)
  • 🟠 Local CLI installs bypass the new node-scoped guidance and af list step (control-plane/internal/core/services/package_service.go:643)
  • 🟠 Git CLI installs omit the af list post-install step (control-plane/internal/packages/git.go:246)
  • … and 8 more (see All Findings by Severity)

Files with findings: control-plane/internal/cli/config.go, control-plane/internal/cli/show_requirements.go, control-plane/internal/core/services/package_service.go, control-plane/internal/packages/env_resolver.go, control-plane/internal/packages/git.go, control-plane/internal/packages/inspect.go

All Findings by Severity

🟠 Important (15)

  • af config writes global declarations into the node scope control-plane/internal/cli/config.go:348
  • af config does not support require_one_of options control-plane/internal/cli/config.go:302
  • Missing-secret repair commands ignore declaration scope control-plane/internal/packages/env_resolver.go:285
  • Local CLI installs bypass the new node-scoped guidance and af list step control-plane/internal/core/services/package_service.go:643
  • Git CLI installs omit the af list post-install step control-plane/internal/packages/git.go:246
  • Local af install bypasses the updated post-install guidance control-plane/internal/core/services/package_service.go:207
  • Local production installs retain unscoped, incomplete post-install guidance control-plane/internal/core/services/package_service.go:649
  • .env writer escapes quotes that its consumers do not unescape control-plane/internal/cli/config.go:344
  • af config --set ignores require_one_of variables and manifest secret scope control-plane/internal/cli/config.go:304
  • Requirements report drops the manifest variable's optional field control-plane/internal/cli/show_requirements.go:85
  • Roll back .env when the secret-store mirror fails control-plane/internal/cli/config.go:344
  • Unset exposes a same-key global secret control-plane/internal/cli/config.go:415
  • Inspection and installation disagree for Git-looking local directories control-plane/internal/packages/inspect.go:26
  • Commit-SHA Git sources fail because they are passed to git clone --branch control-plane/internal/packages/git.go:267
  • Git inspection allows a selected subdirectory to escape the clone through a symlink control-plane/internal/packages/inspect.go:54
Review Process Details

Dimensions Analyzed (6):

  • Config write-through preserves manifest scope and all runtime requirement forms — 3 file(s)
  • Production install paths reach the updated guidance — 4 file(s)
  • Keep post-install guidance owned by every production install path — 4 file(s)
  • Post-install guidance is correct on the CLI paths users actually execute — 2 file(s)
  • Show-requirements source resolution executes for supported Git refs and subdirectories — 3 file(s)
  • Requirements preview accurately and safely represents an installable source — 2 file(s)

Meta-Dimension Lenses (3):

  • Semantic — 3 dimension(s), 91% coverage confidence
  • Mechanical — 3 dimension(s), 88% coverage confidence
  • Systemic — 2 dimension(s), 91% coverage confidence

Cross-Reference & Adversary Analysis:

  • 10 finding(s) adversarially tested: 8 confirmed, 2 challenged
Pipeline Stats
Metric Value
Duration 1375.0s
Agent invocations 32
Coverage iterations 1
Estimated cost N/A (provider does not report cost)
Budget exhausted No
PR type bug_fix
Complexity medium

Review ID: rev_8215276ccecf


return err
}

// Write-through to the node-scoped encrypted secret store so `af run` — which

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Advisory — non-blocking. Safe to merge and address in a follow-up.

Why non-blocking: This is a configuration scope misbehavior that can be corrected later; it does not cause data loss, security issues, or break an existing contract.

Pass the matched UserEnvironmentVar through SetVariable/UnsetVariable and write to envVar.SecretScope(packageName) instead of hard-coding the node scope.

af config --set always calls setNodeSecret (config.go:353), storing the value in the node scope regardless of the declared scope. This creates a node override that masks later global updates, so af secrets set KEY ... no longer reaches this node. Interactive config and --unset have the same bug.

🟠 IMPORTANT · confidence 99%

Evidence

Step 1: A manifest declares API_KEY in required without scope, and UserEnvironmentVar.SecretScope("demo") returns global unless scope is exactly node (installer.go:66-70).
Step 2: af config demo --set API_KEY=value reaches SetVariable, finds the required declaration, and calls setNodeSecret(packageName, key, value) at config.go:353.
Step 3: setNodeSecret calls store.Set(packageName, key, value), therefore storing API_KEY in the demo scope rather than global (config.go:358-364). InteractiveConfig similarly calls writeNodeSecrets(packageName, envVars) (config.go:164), and UnsetVariable deletes from packageName (config.go:415).
Step 4: At run time, EnvResolver.lookup calls Store.Get(r.NodeName, v.Name) (env_resolver.go:145-157), and SecretStore.Get returns a node value before consulting global (secrets.go:193-210). Thus the newly created node value masks later global updates for this node.

🤖 Reviewed by AgentField PR-AF

// exact command that fixes it — `af secrets set <VAR> --node <name>` — so a
// non-interactive startup failure tells the operator what to run, naming the
// node so the command is copy-pasteable.
func missingEnvError(nodeName string, missing []string, groups []RequireOneOfGroup) error {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Advisory — non-blocking. Safe to merge and address in a follow-up.

Why non-blocking: The repair command output being suboptimal for noninteractive runs is a usability issue but does not break the build, cause security vulnerabilities, data loss, or break an existing public API contract.

Missing-secret repair commands ignore declaration scope

🟠 IMPORTANT · confidence 99%

Fix the noninteractive repair command to respect declaration scope. Currently, missingEnvError always generates af secrets set <name> --node <node>, even for global declarations. This silently creates a node-scoped secret that overrides the global value, breaking intended sharing semantics and masking future global updates.

Evidence

Step 1: A required declaration has omitted scope, so runtime interactive prompting persists it with v.SecretScope(r.NodeName), which is global (env_resolver.go:164-177; installer.go:66-70).
Step 2: In a noninteractive run, Resolve records only the variable name in missing (env_resolver.go:39-57) and calls missingEnvError (env_resolver.go:132-133).
Step 3: missingEnvError constructs every fix as af secrets set <name> --node <node> without receiving or examining the declaration scope (env_resolver.go:285-306).
Step 4: Running that displayed command stores a node-scoped value, and SecretStore.Get gives it precedence over global (secrets.go:193-210), unlike the global persistence path the interactive resolver would use.

💡 Suggested Fix

Retain UserEnvironmentVar (rather than only names) when collecting missing requirements and render af secrets set KEY for global declarations, adding --node <node> only when SecretScope(node) is node. Apply the same logic to each require_one_of option and add tests for omitted/global and explicit node scopes.


Scope-correct runtime repair commands · confidence 99%

🤖 Reviewed by AgentField PR-AF

// Save environment file
return pcm.saveEnvFile(packagePath, envVars)
// Persist to the package .env (read by `af dev` and the web UI env editor).
if err := pcm.saveEnvFile(packagePath, envVars); err != nil {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Advisory — non-blocking. Safe to merge and address in a follow-up.

Why non-blocking: The inconsistency is recoverable and does not cause production harm; the runtime uses the secret store's old value, and the error is returned to the user.

Make all .env readers unescape \" to match the writer's escaping.

🟠 IMPORTANT · confidence 99%

saveEnvFile wraps values containing " in quotes and escapes internal " as \", but af dev, the web editor, and PackageConfigManager.loadEnvFile only strip the outer quotes without unescaping, so a value like a"b becomes a\"b. Embedded newlines are likewise corrupted.

Evidence

Writer, control-plane/internal/cli/config.go:500-505:
if strings.ContainsAny(value, " \t\n\r\"'\\$") {
value = fmt.Sprintf("\"%s\"", strings.ReplaceAll(value, "\"", "\\\""))
}
lines = append(lines, fmt.Sprintf("%s=%s", key, value))

af dev reader, control-plane/internal/core/services/dev_service.go:410-429:
lines := strings.Split(string(data), "\n")
value := strings.TrimSpace(parts[1])
if (strings.HasPrefix(value, "\"") && strings.HasSuffix(value, "\"")) || ... {
value = value[1 : len(value)-1]
}

The web GET editor reader has the same logic at control-plane/internal/handlers/ui/env.go:288-303, and PackageConfigManager.loadEnvFile has the same split-and-strip logic at control-plane/internal/cli/config.go:465-488. None unescapes \" or preserves embedded newlines.

💡 Suggested Fix

Use one shared .env encoder/decoder, or make saveEnvFile emit escapes that all three readers explicitly unescape (including backslashes and newlines), with round-trip tests for quotes, backslashes, and multiline values.


Consistency Verifier · confidence 99%

🤖 Reviewed by AgentField PR-AF

return cmd
}

func toRequirementVar(v packages.UserEnvironmentVar) requirementVar {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Advisory — non-blocking. Safe to merge and address in a follow-up.

Why non-blocking: Missing field in output does not break build, security, data, or existing API contracts; it is a completeness gap that can be addressed later.

Add Optional to requirementVar so the field survives serialization.

The YAML manifest declares UserEnvironmentVar.Optional, but toRequirementVar never copies it into requirementVar. That means any explicit optional: true/false is dropped before JSON output and text rendering, so downstream consumers can't reconstruct the original declaration.

Evidence

Parser contract at control-plane/internal/packages/installer.go:54-62: Optional bool yaml:"optional"`` is a field of UserEnvironmentVar. Changed conversion at control-plane/internal/cli/show_requirements.go:85-93 copies only `Name`, `Description`, `Type`, `Default`, `Validation`, and `Scope`; it has no `Optional` field to copy. The output shape at lines 12-20 likewise declares no JSON `optional` field.

💡 Suggested Fix

Add an Optional bool json:"optional"`` field to requirementVar and assign `Optional: v.Optional` in `toRequirementVar`; add a JSON and text contract test using a manifest variable that explicitly declares this field.


Consistency Verifier · confidence 99%

🤖 Reviewed by AgentField PR-AF

// Save environment file
return pcm.saveEnvFile(packagePath, envVars)
// Persist to the package .env (read by `af dev` and the web UI env editor).
if err := pcm.saveEnvFile(packagePath, envVars); err != nil {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Advisory — non-blocking. Safe to merge and address in a follow-up.

Why non-blocking: The inconsistency is recoverable and does not cause production harm; the runtime uses the secret store's old value, and the error is returned to the user.

Roll back .env when the secret-store update fails.

🟠 IMPORTANT · confidence 98%

SetVariable writes the package .env before updating the secret store, but does not roll back if the store update fails. This leaves the new value in .env while the secret store retains the old value, causing subsequent agent runs to use the stale secret despite the .env editor showing the update.

Evidence

Changed location, control-plane/internal/cli/config.go:344-353:
if err := pcm.saveEnvFile(packagePath, envVars); err != nil { return err }
...
return pcm.setNodeSecret(packageName, key, value).

Persistence implementation, control-plane/internal/cli/config.go:508-509:
content := strings.Join(lines, "\n") + "\n"
return os.WriteFile(envPath, []byte(content), 0600).

Secret-store implementation, control-plane/internal/packages/secrets.go:158-164:
values, err := s.load(scope)
if err != nil { return err }
values[key] = value
return s.save(scope, values).
Its save can return an error from os.WriteFile (lines 151-152). No rollback exists in the call site or store.

Subsequent read path, control-plane/internal/packages/env_resolver.go:40-43 specifies resolution from process environment then node/global secret store then defaults; lookup calls r.Store.Get at lines 149-152. The runner creates this resolver in control-plane/internal/packages/runner.go:287-292, without reading the package .env.

💡 Suggested Fix

Make the dual write transactional from the caller's perspective: preserve the prior .env content and restore it if setNodeSecret fails, or write the secret store first and only replace .env after it succeeds. Add a failure-injection test asserting the two persisted destinations remain aligned when SecretStore.Set fails.


Consistency Verifier · confidence 98%

🤖 Reviewed by AgentField PR-AF

if err != nil {
return fmt.Errorf("failed to open secret store: %w", err)
}
if err := store.Delete(packageName, key); err != nil {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Advisory — non-blocking. Safe to merge and address in a follow-up.

Why non-blocking: Unset removes the node-scoped override, reverting to the global default; this is a design choice, not a demonstrated security vulnerability or data loss.

🟠 IMPORTANT · confidence 98%

Make UnsetVariable write a tombstone that prevents SecretStore.Get from falling back to the global scope.

After UnsetVariable deletes the node‑scoped key, SecretStore.Get re‑reads the global value because the node lookup is absent. This means the unset command silently reverts to a same‑key global secret, contradicting its intended effect.

Evidence

Changed end (control-plane/internal/cli/config.go): store.Delete(packageName, key) (line 415). The corresponding set path writes store.Set(packageName, key, value) (line 363).

Other end (control-plane/internal/packages/secrets.go): Delete removes the key from only its supplied scope: delete(values, key) followed by return s.save(scope, values) (lines 181-190). Get checks the node first, but if absent it always loads global: if v, ok := nodeVals[key]; ok { return v, true, nil } followed by globalVals, err := s.load(globalScope) and v, ok := globalVals[key]; return v, ok, nil (lines 195-210).

💡 Suggested Fix
Store an explicit tombstone (e.g., a sentinel value) for the key in the node scope after deletion. Update SecretStore.Get to recognise the tombstone and return false without loading the global scope. Alternatively, make Get treat any node‑level existence check as definitive and skip the global fallback entirely.

🤖 Reviewed by AgentField PR-AF

if src == "" {
return nil, fmt.Errorf("no source provided")
}
if info, err := os.Stat(src); err == nil && info.IsDir() {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Advisory — non-blocking. Safe to merge and address in a follow-up.

Why non-blocking: Inconsistency does not break builds, security, data, or public API contracts; it is a non-blocking functional improvement.

Make install source resolution check for an existing local directory before Git detection

🟠 IMPORTANT · confidence 98%

InspectSource checks for a local directory before IsGitURL, but Install does the reverse. So a local path like github.com/... or *.git that matches IsGitURL is inspected as a directory but installed as a Git clone.

Evidence

Changed location, control-plane/internal/packages/inspect.go:26-30: if info, err := os.Stat(src); err == nil && info.IsDir() { return ParsePackageMetadata(src) } followed by if IsGitURL(src) { return inspectGitSource(src) }. Install resolution, control-plane/internal/core/services/package_service.go:60-71: if packages.IsGitURL(source) { ... return installer.InstallFromGit(source, options.Force) } followed by local installation only afterward. IsGitURL in control-plane/internal/packages/git.go:51-59 classifies strings containing github.com, gitlab.com, bitbucket.org, git., with prefix git@, suffix .git, or matching its broad HTTPS predicate as Git URLs.


Consistency Verifier · confidence 98%

🤖 Reviewed by AgentField PR-AF

}
defer func() { _ = os.RemoveAll(tempDir) }()

dir, err := findManifestDir(tempDir, info.Subdir)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Must-fix before merge. Path traversal via symlink allows reading arbitrary files outside the intended clone root, posing an information disclosure risk.

Fix the symlink resolution in findManifestDir to prevent subdirectory escapes
🟠 IMPORTANT · confidence 93%

The containment check in ResolvePackageSubdir is lexical; it then uses os.Stat on the joined target, following symlinks. A repository can contain a selected subdirectory that is a symlink outside the clone, causing inspection to parse a manifest outside the intended root.

Evidence

Changed end: inspectGitSource calls dir, err := findManifestDir(tempDir, info.Subdir) at inspect.go:54, and findManifestDir calls ResolvePackageSubdir(root, subdir) at inspect.go:67. Other end: ResolvePackageSubdir constructs target := filepath.Join(root, filepath.Clean(subdir)) and checks filepath.Rel(root, target) (installer.go:981-986), but then performs os.Stat(filepath.Join(target, "agentfield-package.yaml")) (installer.go:988-990), which follows a symlink at target. Thus the lexical target remains under root while its resolved filesystem location may be outside it.

💡 Suggested Fix

Before accepting a selected subdirectory, resolve symlinks for the clone root and target (or reject symlinked selector directories), then verify the resolved target remains under the resolved clone root before checking or parsing its manifest.


Consistency Verifier · confidence 93%

🤖 Reviewed by AgentField PR-AF

@CLAassistant

CLAassistant commented Jul 24, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

AbirAbbas and others added 2 commits July 24, 2026 13:21
The symlink-containment hardening made EvalSymlinks fail first on absent
subdirectories, surfacing a raw lstat error instead of the documented
'no agentfield-package.yaml found (expected at <path>)' message and
breaking four tests that pin that contract. Handle not-exist explicitly
before the resolution error path.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AbirAbbas
AbirAbbas force-pushed the fix/config-secrets-coherence branch from 5d37359 to efa425b Compare July 24, 2026 17:21
…paths

Closes the patch-coverage gap: no-configuration and unlabeled
require_one_of rendering plus inspect-error propagation in
af show-requirements, and the root-resolution / manifest-missing error
branches of ResolvePackageSubdir.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AbirAbbas
AbirAbbas merged commit 4a64cde into main Jul 24, 2026
25 checks passed
@AbirAbbas
AbirAbbas deleted the fix/config-secrets-coherence branch July 24, 2026 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants