internal/secret/secret.go (1)

208-222: ⚡ Quick win

Verify HMAC fingerprint behavior for empty inputs.

When providerRef is empty (line 212-214), the function returns an empty fingerprint without error. However, when the key is empty (line 209-211), it returns an error. This asymmetry may be intentional—empty ref suggests no provider connection, while empty key is a configuration error—but it should be verified.

Additionally, the HMAC construction includes null-byte separators (lines 217, 219) to prevent collision attacks where different field combinations could produce the same MAC. This is good practice.

Consider adding a test case that explicitly validates the empty providerRef behavior returns ("", nil) to document this design choice.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/secret/secret.go` around lines 208 - 222, Add unit tests for
ProviderRefFingerprint to document and lock in the current asymmetrical
behavior: create a test that calls ProviderRefFingerprint with a valid non-empty
key and providerType/providerConnectionID but an empty providerRef and assert it
returns ("", nil), and a test that calls it with an empty key and non-empty
providerRef and asserts an error is returned; reference the
ProviderRefFingerprint function and its parameters (key, providerType,
providerConnectionID, providerRef) so reviewers can see the intended behavior is
explicit.

internal/persis/filesecret/store.go (2)

89-119: ⚡ Quick win

Consider logging corruption details for investigation.

During index rebuild (lines 107-111), corrupted secret files are logged as warnings but silently skipped. While this prevents startup failure, it might hide data integrity issues. Consider adding more diagnostic information to the log message, such as the specific parsing error or file size, to aid debugging.

💡 Enhanced logging for troubleshooting

 		slog.Warn("Failed to load secret file during index rebuild",
 			slog.String("file", filePath),
+			slog.Int64("size", entry.Size()),
 			slog.String("error", err.Error()))

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/persis/filesecret/store.go` around lines 89 - 119, The rebuildIndex
function currently warns on failed loads but omits useful corruption details;
update the slog.Warn call inside the loop (where loadRecordFromFile returns err)
to include the actual parsing error value (err), the file size (via
os.Stat(filePath).Size()), and optionally a short hex checksum (e.g., sha256 of
the file bytes) to aid investigation; use the existing symbols
loadRecordFromFile, slog.Warn, filePath, and refKey context when adding these
fields so s.byID and s.byRef logic remains unchanged.

---

331-361: ⚖️ Poor tradeoff

Timestamp updates during ResolveValue may cause contention.

ResolveValue updates LastResolvedAt and UpdatedAt (lines 355-356) and writes the record back to disk (line 357) on every resolution. For frequently accessed secrets, this causes unnecessary write amplification and file-lock contention. Consider whether these timestamp updates should be:

1. Asynchronous/batched to reduce I/O
2. Periodic rather than on every access
3. Optional based on configuration

This is a design tradeoff: precise access tracking vs. performance. If audit trails require exact resolution timestamps, the current approach is correct but expensive.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/persis/filesecret/store.go` around lines 331 - 361, ResolveValue
currently updates record.Secret.LastResolvedAt and UpdatedAt and calls
writeRecordToFile on every access, which causes write amplification; modify
Store.ResolveValue (and the Store struct) so timestamp writes are optional or
deferred: add a configuration flag or enum on Store (e.g.,
UpdateResolveTimestamps bool or ResolveUpdateStrategy {Immediate, Async,
Periodic, Disabled}), then in ResolveValue only update and call
writeRecordToFile when strategy == Immediate; for Async spawn a non-blocking
goroutine or enqueue an update to an internal worker that serializes writes
(ensure you avoid holding s.mu while doing async I/O), for Periodic record the
last-resolved time in-memory and flush it on a timer or during other mutations;
reference the ResolveValue method, record.Secret.LastResolvedAt/UpdatedAt, and
writeRecordToFile when implementing the conditional or deferred update handling.

internal/cmn/secrets/resolver.go (1)

154-156: 💤 Low value

Consider extracting registry ref validation to reduce duplication.

The validation logic checking that registry refs don't include provider/key/options appears in both Resolve (lines 155-156) and CheckAccessibility (lines 220-221). Consider extracting to a helper method for maintainability.

♻️ Example refactor

+func validateRegistryRef(ref core.SecretRef) error {
+	if ref.Provider != "" || ref.Key != "" || len(ref.Options) > 0 {
+		return fmt.Errorf("secret %q registry ref cannot include provider, key, or options", ref.Name)
+	}
+	return nil
+}
+
 func (r *Registry) Resolve(ctx context.Context, ref core.SecretRef) (string, error) {
 	if ref.Ref != "" {
-		if ref.Provider != "" || ref.Key != "" || len(ref.Options) > 0 {
-			return "", fmt.Errorf("secret %q registry ref cannot include provider, key, or options", ref.Name)
-		}
+		if err := validateRegistryRef(ref); err != nil {
+			return "", err
+		}

Also applies to: 220-221

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cmn/secrets/resolver.go` around lines 154 - 156, Extract the
duplicated registry ref validation into a shared helper (e.g.,
validateRegistryRef or SecretRef.ValidateRegistryRef) and call it from both
Resolve and CheckAccessibility: move the logic that checks if ref.Ref != "" then
ensures ref.Provider == "" && ref.Key == "" && len(ref.Options) == 0, returning
the same error fmt.Errorf("secret %q registry ref cannot include provider, key,
or options", ref.Name) when violated; replace the original inline checks in
Resolve and CheckAccessibility with calls to this new helper to remove
duplication and keep behavior identical.

internal/service/frontend/api/v1/secrets.go (2)

396-396: ⚖️ Poor tradeoff

Semantic coupling: secret management reuses audit permission check.

The secret management permission check uses role.CanManageAudit() (line 396), which semantically grants Manager+ roles. This works today since both audit and secrets require Manager+ access, but creates coupling between two unrelated features. If secret permissions ever need to diverge from audit permissions (e.g., a dedicated "secret manager" role), this logic will need refactoring.

Consider adding a role.CanManageSecrets() method in the domain layer that currently delegates to CanManageAudit(), making future permission changes easier to implement.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/frontend/api/v1/secrets.go` at line 396, The permission
check for secret management currently calls role.CanManageAudit(), coupling
secrets to audit; add a new domain method role.CanManageSecrets() that for now
returns role.CanManageAudit() (delegate) and replace the call in
internal/service/frontend/api/v1/secrets.go (the if !role.CanManageAudit()
check) with if !role.CanManageSecrets() so future permission changes for secrets
can be implemented by changing CanManageSecrets() only.

---

84-106: 💤 Low value

Consider store-level pagination for large secret registries.

The current implementation fetches all secrets, filters by workspace permission, then paginates in-memory. For deployments with thousands of secrets across many workspaces, this pattern could consume significant memory and CPU. Consider adding offset/limit to the ListOptions and pushing pagination into the store layer if performance becomes a concern.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/frontend/api/v1/secrets.go` around lines 84 - 106, The code
currently loads all secrets then paginates in-memory; change this to push
pagination into the store by adding Offset and Limit to secretpkg.ListOptions
and passing the request offset/limit into a.secretStore.List(ctx,
secretpkg.ListOptions{Workspace: workspaceFilter, Offset: offset, Limit:
limit}); update the secret store implementations to apply offset/limit at the
DB/query layer and (optionally) return a total count so the handler can set
total without slicing; after that remove the in-memory slicing logic around
visible (keep the permission check via a.canManageSecretWorkspace and mapping
via toSecretResponse) and rely on the store to return only the requested page.

internal/service/frontend/api/v1/secrets_test.go (1)

21-91: 🏗️ Heavy lift

Consider expanding test coverage in follow-up work.

The current tests verify critical security properties (write-only values, duplicate rejection), but several scenarios remain untested:

• Update, Delete, Enable, Disable operations
• Permission enforcement (workspace access, manager role requirement)
• Workspace filtering and pagination edge cases
• External provider secrets (non-DaguManaged flows)
• Error handling (invalid provider type, empty values, malformed requests)

Since this is a new feature, having smoke tests for the critical security property is acceptable for initial merge. Consider adding comprehensive test coverage as the feature matures.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/frontend/api/v1/secrets_test.go` around lines 21 - 91, Add
follow-up tests expanding coverage around secrets: create new test functions
alongside TestSecretsAPI_CreateWriteListDoesNotReturnPlaintext and
TestSecretsAPI_CreateRejectsDuplicateWorkspaceRef to cover UpdateSecret,
DeleteSecret, EnableSecret, DisableSecret flows (exercise api.UpdateSecret,
api.DeleteSecret, api.EnableSecret/DisableSecret), permission enforcement
(workspace access and manager role checks when calling
CreateSecret/WriteSecretVersion/ListSecrets), workspace filtering and pagination
edge cases for ListSecrets, external provider flows (use non-DaguManaged
provider types such as SecretProviderTypeExternal and provider-specific
handling), and error cases (invalid provider type, empty/malformed values) while
reusing store.ResolveValue to assert secret material is never present in API
responses; implement these as separate focused tests in
internal/service/frontend/api/v1/secrets_test.go.

ui/src/pages/secrets/index.tsx (1)

575-576: ⚡ Quick win

Use compact control heights to match the frontend density guideline.

New form/select inputs use h-9; this page should use the compact sizing pattern (h-7 or smaller for selects/inputs).

As per coding guidelines: “Use information-dense, compact UI design… small form element heights (h-7 or smaller for select boxes)”.

Also applies to: 593-594, 618-619, 636-637, 655-656, 670-671, 776-777

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/pages/secrets/index.tsx` around lines 575 - 576, Replace the oversized
form control height classes (className="h-9") in ui/src/pages/secrets/index.tsx
with the compact size (use className="h-7" or smaller) for all input/select
components to follow the dense UI guideline; search for every occurrence of
className="h-9" in this file (including the instances around the current diff
and the other reported spots) and update those input/select/select-like
components (the JSX elements that render form controls) to use h-7 so selects
and inputs render with the compact height.

ui/src/__tests__/menu.test.tsx (1)

132-144: ⚡ Quick win

Add explicit assertions for Secrets nav visibility (allowed vs denied).

The new useCanManageSecrets path is mocked but not asserted. Please add one test where it’s true (Secrets link visible) and one where it’s false (hidden).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/src/__tests__/menu.test.tsx` around lines 132 - 144, The test suite's
beforeEach sets useCanManageSecretsMock but there are no explicit tests
asserting the Secrets navigation visibility; add two tests in
ui/src/__tests__/menu.test.tsx that toggle useCanManageSecretsMock to true and
false and assert the Secrets link appears or is absent accordingly.
Specifically, create one test that sets
useCanManageSecretsMock.mockReturnValue(true), renders the menu (using the
existing render helper in the file), and expects a DOM node with text "Secrets"
(or the same selector used for other nav links) to be in the document; create
another test that sets useCanManageSecretsMock.mockReturnValue(false), renders
the menu, and expects that same "Secrets" node is not present. Use the existing
beforeEach and other auth mocks to keep context consistent and reference the
useCanManageSecretsMock and the menu render helper when adding the tests.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/core/spec/builder.go (1)

212-230: ⚖️ Poor tradeoff

Consider adding a test to enforce synchronization between this list and internal/core/exec constants.

The reserved names list is currently in sync with internal/core/exec/env.go, but there's no enforcement mechanism to prevent divergence if env keys are added, removed, or renamed in the future. A test that validates this list against the actual constants in internal/core/exec would prevent collision bugs from silent mismatches.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/core/spec/builder.go` around lines 212 - 230, Add a unit test that
fails if the reservedSecretEnvNames slice diverges from the env key constants
defined in the internal/core/exec package: write a test (e.g.,
TestReservedSecretEnvNamesMatchExecConstants) that imports internal/core/exec,
collects the exported env key constants from that package into a set, and
compares that set to the reservedSecretEnvNames slice; if there is any missing
or extra name, the test should fail with a clear message. Ensure the test
references the reservedSecretEnvNames symbol and the exec package constants so
future edits to either will be caught.

internal/service/frontend/api/v1/secrets.go (2)

383-395: ⚡ Quick win

Clarify why secret management requires audit permission.

The method name CanManageAudit() suggests it checks audit log management capability, but here it gates access to secret management operations. Either add a comment explaining this permission mapping or introduce a more semantically appropriate check like role.CanManageSecrets() if available.

💡 Suggested comment to add

 func (a *API) requireSecretManageForWorkspace(ctx context.Context, workspaceName string) error {
 	role, ok, err := a.effectiveRoleForWorkspace(ctx, workspaceName)
 	if err != nil {
 		return err
 	}
 	if !ok {
 		return workspaceResourceNotFound()
 	}
+	// Secret management requires audit management capability (manager role).
 	if !role.CanManageAudit() {
 		return errInsufficientPermissions
 	}
 	return nil
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/frontend/api/v1/secrets.go` around lines 383 - 395, The
permission check in requireSecretManageForWorkspace uses role.CanManageAudit(),
which is confusing because it suggests audit-log permissions are being used to
gate secret operations; either replace that call with a semantically correct
permission check (e.g., role.CanManageSecrets()) if such a method exists on the
Role type, or if business logic intentionally reuses the audit permission to
control secret management, add a concise comment above
requireSecretManageForWorkspace explaining that mapping and why CanManageAudit()
is the authoritative gate for secret management to avoid confusion for future
readers.

---

64-107: ⚡ Quick win

Potentially redundant permission filter in ListSecrets.

Line 73 validates that the user can manage the workspace specified in workspaceFilter, and line 82 lists secrets scoped to that workspace. The subsequent per-secret permission check on lines 88-93 should always succeed since all returned secrets belong to the already-validated workspace. Consider removing the redundant filter or adding a comment if this is intentional defensive programming.

♻️ Option 1: Remove redundant filter

 	secrets, err := a.secretStore.List(ctx, secretpkg.ListOptions{Workspace: workspaceFilter})
 	if err != nil {
 		return nil, fmt.Errorf("failed to list secrets: %w", err)
 	}

-	visible := make([]api.SecretResponse, 0, len(secrets))
+	visible := make([]api.SecretResponse, len(secrets))
 	for _, sec := range secrets {
-		if !a.canManageSecretWorkspace(ctx, sec.Workspace) {
-			continue
-		}
-		visible = append(visible, toSecretResponse(sec))
+		visible[i] = toSecretResponse(sec)
 	}

 	total := len(visible)

Option 2: Add clarifying comment if intentional

 	secrets, err := a.secretStore.List(ctx, secretpkg.ListOptions{Workspace: workspaceFilter})
 	if err != nil {
 		return nil, fmt.Errorf("failed to list secrets: %w", err)
 	}

+	// Defensive filter: although workspaceFilter was already validated,
+	// re-check permissions for each secret to guard against store bugs.
 	visible := make([]api.SecretResponse, 0, len(secrets))
 	for _, sec := range secrets {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/frontend/api/v1/secrets.go` around lines 64 - 107,
ListSecrets performs requireSecretManageForWorkspace after
parseSecretWorkspaceSelector and then calls a.secretStore.List scoped by that
workspace, so the per-secret permission check using a.canManageSecretWorkspace
inside the loop is redundant; either remove that check and directly append to
visible using toSecretResponse(secrets...), or if you intended defensive
programming, keep the check but add a clarifying comment above the loop
explaining why per-secret permission re-check is required, referencing the
ListSecrets method and the requireSecretManageForWorkspace, secretStore.List,
and canManageSecretWorkspace symbols so future readers understand the intent.