[codex] Fix local skill, secrets, and file viewer regressions (#8586)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Agents rely on local skills, provider-backed secrets, and workspace file previews during normal execution. > - Local skill imports need bounded reference-file inventory so direct skill discovery stays accurate without accidentally walking too much of the filesystem. > - Secrets provider setup needs actionable AWS discovery errors so operators can recover from IAM/config problems without losing manual form input. > - The issue detail file viewer should reopen cleanly after the first close so users can keep inspecting files during task review. > - This pull request collects the small fixes and regression tests for those related operator workflows. > - The benefit is more predictable local skill imports, clearer secrets setup failure states, and a less brittle file preview interaction. ## Linked Issues or Issue Description - No public GitHub issue was found for this extracted local work. - Related prior sync context: #8536. - Problem: local skill reference discovery, AWS provider-vault discovery errors, and issue file preview reopening each had narrow workflow regressions that made operator recovery harder. - Expected behavior: skill imports inventory reference files within bounded local skill directories, AWS discovery failures present safe actionable guidance while preserving manual values, and closing the first file preview does not prevent opening another preview. - Reproduction scope: import a local skill with referenced files, attempt AWS Secrets Manager discovery with insufficient IAM/list permissions, and open/close/reopen file previews from an issue detail page. - Duplicate search: searched GitHub PRs/issues for `skill inventory secrets file viewer` and `skill inventory secrets AWS file viewer`; no matching public duplicate was found. ## What Changed - Bounded direct local skill file inventory discovery and added regression coverage for reference file imports. - Preserved and surfaced safe, actionable AWS Secrets Manager discovery/import errors in server responses and the secrets UI. - Kept AWS provider-vault manual form values intact when discovery fails or returns no candidates. - Fixed issue file viewer state so closing the first preview still allows later file previews to open. - Updated the secrets render test harness to avoid the missing `React.act` export in the current React package set. ## Verification - `pnpm run preflight:workspace-links && pnpm exec vitest run server/src/__tests__/company-skills-service.test.ts server/src/__tests__/secrets-routes.test.ts server/src/__tests__/secrets-service.test.ts ui/src/context/FileViewerContext.test.ts ui/src/pages/Secrets.render.test.tsx` - Result: 5 test files passed, 116 tests passed. - Install note: the isolated worktree needed `NODE_ENV=development pnpm install --frozen-lockfile --prod=false --force` before local verification because it initially had no dev dependencies installed. ## Risks - Low-to-medium risk: this touches skill import inventory, secrets-provider error handling, and file-viewer UI state, but each change is covered by focused regression tests. - No migrations. - No dependency or lockfile changes. - CI is rerunning on the latest head after review fixes; Greptile is 5/5 with no unresolved Greptile threads. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - OpenAI Codex coding agent, GPT-5-family model as provided in the Paperclip run environment, with repository tool use and local command execution. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
Dotta
GitHub
parent
ef37203a48
commit
f88ac9d078
|
|
@ -577,6 +577,116 @@ describeEmbeddedPostgres("companySkillService.list", () => {
|
|||
await expect(svc.getById(companyId, skillId)).resolves.toBeNull();
|
||||
});
|
||||
|
||||
it("refreshes stale local-path file inventory from disk", async () => {
|
||||
const companyId = randomUUID();
|
||||
const skillId = randomUUID();
|
||||
const skillDir = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-stale-inventory-skill-"));
|
||||
cleanupDirs.add(skillDir);
|
||||
await fs.mkdir(path.join(skillDir, "references"), { recursive: true });
|
||||
await fs.writeFile(path.join(skillDir, "SKILL.md"), "# Stale Inventory Skill\n", "utf8");
|
||||
await fs.writeFile(path.join(skillDir, "references", "guide.md"), "# Guide\n", "utf8");
|
||||
|
||||
await db.insert(companies).values({
|
||||
id: companyId,
|
||||
name: "Paperclip",
|
||||
issuePrefix: `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`,
|
||||
requireBoardApprovalForNewAgents: false,
|
||||
});
|
||||
await db.insert(companySkills).values({
|
||||
id: skillId,
|
||||
companyId,
|
||||
key: `company/${companyId}/stale-inventory-skill`,
|
||||
slug: "stale-inventory-skill",
|
||||
name: "Stale Inventory Skill",
|
||||
description: null,
|
||||
markdown: "# Stale Inventory Skill\n",
|
||||
sourceType: "local_path",
|
||||
sourceLocator: skillDir,
|
||||
trustLevel: "markdown_only",
|
||||
compatibility: "compatible",
|
||||
fileInventory: [{ path: "SKILL.md", kind: "skill" }],
|
||||
metadata: { sourceKind: "local_path" },
|
||||
});
|
||||
|
||||
const listed = await svc.list(companyId);
|
||||
const skill = listed.find((entry) => entry.id === skillId);
|
||||
|
||||
expect(new Set(skill?.fileInventory.map((entry) => `${entry.kind}:${entry.path}`))).toEqual(new Set([
|
||||
"skill:SKILL.md",
|
||||
"reference:references/guide.md",
|
||||
]));
|
||||
await expect(svc.readFile(companyId, skillId, "references/guide.md")).resolves.toMatchObject({
|
||||
path: "references/guide.md",
|
||||
kind: "reference",
|
||||
content: "# Guide\n",
|
||||
});
|
||||
await expect(svc.getById(companyId, skillId)).resolves.toMatchObject({
|
||||
fileInventory: expect.arrayContaining([
|
||||
expect.objectContaining({ path: "SKILL.md", kind: "skill" }),
|
||||
expect.objectContaining({ path: "references/guide.md", kind: "reference" }),
|
||||
]),
|
||||
});
|
||||
});
|
||||
|
||||
it("imports sibling reference files when the source is a direct SKILL.md path", async () => {
|
||||
const companyId = randomUUID();
|
||||
const skillDir = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-file-import-skill-"));
|
||||
cleanupDirs.add(skillDir);
|
||||
await fs.mkdir(path.join(skillDir, "references"), { recursive: true });
|
||||
await fs.writeFile(
|
||||
path.join(skillDir, "SKILL.md"),
|
||||
"---\nname: File Import Skill\n---\n\n# File Import Skill\n",
|
||||
"utf8",
|
||||
);
|
||||
await fs.writeFile(path.join(skillDir, "references", "checklist.md"), "# Checklist\n", "utf8");
|
||||
|
||||
await db.insert(companies).values({
|
||||
id: companyId,
|
||||
name: "Paperclip",
|
||||
issuePrefix: `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`,
|
||||
requireBoardApprovalForNewAgents: false,
|
||||
});
|
||||
|
||||
const result = await svc.importFromSource(companyId, path.join(skillDir, "SKILL.md"));
|
||||
|
||||
expect(result.imported).toHaveLength(1);
|
||||
expect(new Set(result.imported[0]?.fileInventory.map((entry) => `${entry.kind}:${entry.path}`))).toEqual(new Set([
|
||||
"skill:SKILL.md",
|
||||
"reference:references/checklist.md",
|
||||
]));
|
||||
});
|
||||
|
||||
it("bounds direct root SKILL.md imports to known support directories", async () => {
|
||||
const companyId = randomUUID();
|
||||
const repoDir = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-root-skill-"));
|
||||
cleanupDirs.add(repoDir);
|
||||
await fs.mkdir(path.join(repoDir, "references"), { recursive: true });
|
||||
await fs.mkdir(path.join(repoDir, "server", "src"), { recursive: true });
|
||||
await fs.writeFile(
|
||||
path.join(repoDir, "SKILL.md"),
|
||||
"---\nname: Root Skill\n---\n\n# Root Skill\n",
|
||||
"utf8",
|
||||
);
|
||||
await fs.writeFile(path.join(repoDir, "references", "guide.md"), "# Guide\n", "utf8");
|
||||
await fs.writeFile(path.join(repoDir, "README.md"), "# Repo readme\n", "utf8");
|
||||
await fs.writeFile(path.join(repoDir, "server", "src", "index.ts"), "export {};\n", "utf8");
|
||||
|
||||
await db.insert(companies).values({
|
||||
id: companyId,
|
||||
name: "Paperclip",
|
||||
issuePrefix: `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`,
|
||||
requireBoardApprovalForNewAgents: false,
|
||||
});
|
||||
|
||||
const result = await svc.importFromSource(companyId, path.join(repoDir, "SKILL.md"));
|
||||
|
||||
expect(result.imported).toHaveLength(1);
|
||||
expect(result.imported[0]?.fileInventory.map((entry) => entry.path).sort()).toEqual([
|
||||
"SKILL.md",
|
||||
"references/guide.md",
|
||||
]);
|
||||
});
|
||||
|
||||
it("rejects executable external package skills before persistence", async () => {
|
||||
const companyId = randomUUID();
|
||||
await db.insert(companies).values({
|
||||
|
|
|
|||
|
|
@ -236,6 +236,55 @@ describe("secret routes", () => {
|
|||
expect(JSON.stringify(mockLogActivity.mock.calls)).not.toContain("paperclip/prod-use1/company-1/openai");
|
||||
});
|
||||
|
||||
it("returns actionable sanitized provider vault discovery errors", async () => {
|
||||
mockSecretService.previewProviderConfigDiscovery.mockRejectedValue(
|
||||
new HttpError(
|
||||
403,
|
||||
"AWS Secrets Manager denied the request. Check IAM permissions for this provider vault.",
|
||||
{
|
||||
code: "access_denied",
|
||||
provider: "aws_secrets_manager",
|
||||
operation: "secret_provider_config.discovery.preview",
|
||||
providerConfigId: "discovery-preview",
|
||||
providerVaultContext: "draft_config",
|
||||
region: "us-east-1",
|
||||
credentialPath: "Paperclip server runtime/provider credential path",
|
||||
requiredCapability: "secretsmanager:ListSecrets",
|
||||
actionableMessage:
|
||||
"AWS discovery preview needs secretsmanager:ListSecrets in the selected region for the Paperclip server runtime/provider credential path.",
|
||||
safeAlternative:
|
||||
"If the operator already knows the exact AWS Secrets Manager ARN, paste/link that ARN instead of using discovery. Exact-resource DescribeSecret and runtime read permissions are still required.",
|
||||
},
|
||||
),
|
||||
);
|
||||
|
||||
const res = await request(createApp())
|
||||
.post("/api/companies/company-1/secret-provider-configs/discovery/preview")
|
||||
.send({
|
||||
provider: "aws_secrets_manager",
|
||||
config: { region: "us-east-1" },
|
||||
pageSize: 25,
|
||||
});
|
||||
|
||||
expect(res.status).toBe(403);
|
||||
expect(res.body).toMatchObject({
|
||||
error: "AWS Secrets Manager denied the request. Check IAM permissions for this provider vault.",
|
||||
details: {
|
||||
code: "access_denied",
|
||||
provider: "aws_secrets_manager",
|
||||
operation: "secret_provider_config.discovery.preview",
|
||||
providerVaultContext: "draft_config",
|
||||
region: "us-east-1",
|
||||
requiredCapability: "secretsmanager:ListSecrets",
|
||||
},
|
||||
});
|
||||
expect(res.body.details.actionableMessage).toContain("Paperclip server runtime/provider credential path");
|
||||
expect(res.body.details.safeAlternative).toContain("paste/link that ARN");
|
||||
expect(JSON.stringify(res.body)).not.toContain("arn:aws");
|
||||
expect(JSON.stringify(res.body)).not.toContain("123456789012");
|
||||
expect(mockLogActivity).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("rejects ready status for coming-soon provider vaults", async () => {
|
||||
const res = await request(createApp()).post("/api/companies/company-1/secret-provider-configs").send({
|
||||
provider: "vault",
|
||||
|
|
|
|||
|
|
@ -1432,7 +1432,20 @@ describeEmbeddedPostgres("secretService", () => {
|
|||
expect(thrown).toMatchObject({
|
||||
status: 403,
|
||||
message: "AWS Secrets Manager denied the request. Check IAM permissions for this provider vault.",
|
||||
details: { code: "access_denied" },
|
||||
details: {
|
||||
code: "access_denied",
|
||||
provider: "aws_secrets_manager",
|
||||
operation: "secret_provider_config.discovery.preview",
|
||||
providerConfigId: "discovery-preview",
|
||||
providerVaultContext: "draft_config",
|
||||
region: "us-east-1",
|
||||
credentialPath: "Paperclip server runtime/provider credential path",
|
||||
requiredCapability: "secretsmanager:ListSecrets",
|
||||
actionableMessage:
|
||||
"AWS discovery preview needs secretsmanager:ListSecrets in the selected region for the Paperclip server runtime/provider credential path.",
|
||||
safeAlternative:
|
||||
"If the operator already knows the exact AWS Secrets Manager ARN, paste/link that ARN instead of using discovery. Exact-resource DescribeSecret and runtime read permissions are still required.",
|
||||
},
|
||||
});
|
||||
expect(JSON.stringify(thrown)).not.toContain("arn:aws");
|
||||
expect(JSON.stringify(thrown)).not.toContain("123456789012");
|
||||
|
|
|
|||
|
|
@ -939,6 +939,38 @@ async function collectLocalSkillInventory(
|
|||
.sort((left, right) => left.path.localeCompare(right.path));
|
||||
}
|
||||
|
||||
function inventoryEntriesEqual(
|
||||
left: CompanySkillFileInventoryEntry[],
|
||||
right: CompanySkillFileInventoryEntry[],
|
||||
) {
|
||||
if (left.length !== right.length) return false;
|
||||
const normalize = (entries: CompanySkillFileInventoryEntry[]) =>
|
||||
entries
|
||||
.map((entry) => ({
|
||||
path: normalizePortablePath(entry.path),
|
||||
kind: entry.kind,
|
||||
}))
|
||||
.sort((leftEntry, rightEntry) => leftEntry.path.localeCompare(rightEntry.path));
|
||||
const normalizedLeft = normalize(left);
|
||||
const normalizedRight = normalize(right);
|
||||
return normalizedLeft.every((entry, index) => {
|
||||
const other = normalizedRight[index];
|
||||
return other?.path === entry.path && other.kind === entry.kind;
|
||||
});
|
||||
}
|
||||
|
||||
function inferLocalSkillInventoryMode(
|
||||
skill: Pick<CompanySkillRow, "sourceLocator" | "metadata">,
|
||||
): LocalSkillInventoryMode {
|
||||
const metadata = isPlainRecord(skill.metadata) ? skill.metadata : null;
|
||||
const sourceKind = asString(metadata?.sourceKind);
|
||||
const workspaceCwd = asString(metadata?.workspaceCwd);
|
||||
if (sourceKind === "project_scan" && workspaceCwd && skill.sourceLocator === workspaceCwd) {
|
||||
return "project_root";
|
||||
}
|
||||
return "full";
|
||||
}
|
||||
|
||||
export async function readLocalSkillImportFromDirectory(
|
||||
companyId: string,
|
||||
skillDir: string,
|
||||
|
|
@ -1022,8 +1054,9 @@ async function readLocalSkillImports(companyId: string, sourcePath: string): Pro
|
|||
|
||||
if (stat.isFile()) {
|
||||
const markdown = await fs.readFile(resolvedPath, "utf8");
|
||||
const sourceDir = path.dirname(resolvedPath);
|
||||
const parsed = parseFrontmatterMarkdown(markdown);
|
||||
const slug = deriveImportedSkillSlug(parsed.frontmatter, path.basename(path.dirname(resolvedPath)));
|
||||
const slug = deriveImportedSkillSlug(parsed.frontmatter, path.basename(sourceDir));
|
||||
const parsedMetadata = isPlainRecord(parsed.frontmatter.metadata) ? parsed.frontmatter.metadata : null;
|
||||
const skillKey = readCanonicalSkillKey(parsed.frontmatter, parsedMetadata);
|
||||
const metadata = {
|
||||
|
|
@ -1031,14 +1064,12 @@ async function readLocalSkillImports(companyId: string, sourcePath: string): Pro
|
|||
...(parsedMetadata ?? {}),
|
||||
sourceKind: "local_path",
|
||||
};
|
||||
const inventory: CompanySkillFileInventoryEntry[] = [
|
||||
{ path: "SKILL.md", kind: "skill" },
|
||||
];
|
||||
const inventory = await collectLocalSkillInventory(sourceDir, "project_root");
|
||||
return [{
|
||||
key: deriveCanonicalSkillKey(companyId, {
|
||||
slug,
|
||||
sourceType: "local_path",
|
||||
sourceLocator: path.dirname(resolvedPath),
|
||||
sourceLocator: sourceDir,
|
||||
metadata,
|
||||
}),
|
||||
slug,
|
||||
|
|
@ -1047,7 +1078,7 @@ async function readLocalSkillImports(companyId: string, sourcePath: string): Pro
|
|||
markdown,
|
||||
packageDir: path.dirname(resolvedPath),
|
||||
sourceType: "local_path",
|
||||
sourceLocator: path.dirname(resolvedPath),
|
||||
sourceLocator: sourceDir,
|
||||
sourceRef: null,
|
||||
trustLevel: deriveTrustLevel(inventory),
|
||||
compatibility: "compatible",
|
||||
|
|
@ -1237,6 +1268,18 @@ async function readUrlSkillImports(
|
|||
throw unprocessable("Unsupported skill source. Use a local path or URL.");
|
||||
}
|
||||
|
||||
function normalizeFileInventory(row: { fileInventory: unknown }): CompanySkillFileInventoryEntry[] {
|
||||
return Array.isArray(row.fileInventory)
|
||||
? row.fileInventory.flatMap((entry) => {
|
||||
if (!isPlainRecord(entry)) return [];
|
||||
return [{
|
||||
path: String(entry.path ?? ""),
|
||||
kind: (String(entry.kind ?? "other") as CompanySkillFileInventoryEntry["kind"]),
|
||||
}];
|
||||
})
|
||||
: [];
|
||||
}
|
||||
|
||||
function toCompanySkill(row: CompanySkillRow): CompanySkill {
|
||||
return {
|
||||
...row,
|
||||
|
|
@ -1246,15 +1289,7 @@ function toCompanySkill(row: CompanySkillRow): CompanySkill {
|
|||
sourceRef: row.sourceRef ?? null,
|
||||
trustLevel: row.trustLevel as CompanySkillTrustLevel,
|
||||
compatibility: row.compatibility as CompanySkillCompatibility,
|
||||
fileInventory: Array.isArray(row.fileInventory)
|
||||
? row.fileInventory.flatMap((entry) => {
|
||||
if (!isPlainRecord(entry)) return [];
|
||||
return [{
|
||||
path: String(entry.path ?? ""),
|
||||
kind: (String(entry.kind ?? "other") as CompanySkillFileInventoryEntry["kind"]),
|
||||
}];
|
||||
})
|
||||
: [],
|
||||
fileInventory: normalizeFileInventory(row),
|
||||
iconUrl: row.iconUrl ?? null,
|
||||
color: row.color ?? null,
|
||||
tagline: row.tagline ?? null,
|
||||
|
|
@ -1282,15 +1317,7 @@ function toCompanySkillListRow(row: CompanySkillListDbRow): CompanySkillListRow
|
|||
sourceRef: row.sourceRef ?? null,
|
||||
trustLevel: row.trustLevel as CompanySkillTrustLevel,
|
||||
compatibility: row.compatibility as CompanySkillCompatibility,
|
||||
fileInventory: Array.isArray(row.fileInventory)
|
||||
? row.fileInventory.flatMap((entry) => {
|
||||
if (!isPlainRecord(entry)) return [];
|
||||
return [{
|
||||
path: String(entry.path ?? ""),
|
||||
kind: (String(entry.kind ?? "other") as CompanySkillFileInventoryEntry["kind"]),
|
||||
}];
|
||||
})
|
||||
: [],
|
||||
fileInventory: normalizeFileInventory(row),
|
||||
iconUrl: row.iconUrl ?? null,
|
||||
color: row.color ?? null,
|
||||
tagline: row.tagline ?? null,
|
||||
|
|
@ -2137,6 +2164,8 @@ export function companySkillService(db: Db) {
|
|||
slug: companySkills.slug,
|
||||
sourceType: companySkills.sourceType,
|
||||
sourceLocator: companySkills.sourceLocator,
|
||||
trustLevel: companySkills.trustLevel,
|
||||
fileInventory: companySkills.fileInventory,
|
||||
metadata: companySkills.metadata,
|
||||
})
|
||||
.from(companySkills)
|
||||
|
|
@ -2144,6 +2173,8 @@ export function companySkillService(db: Db) {
|
|||
const skills = rows.map((row) => ({
|
||||
...row,
|
||||
sourceType: row.sourceType as CompanySkillSourceType,
|
||||
trustLevel: row.trustLevel as CompanySkillTrustLevel,
|
||||
fileInventory: normalizeFileInventory(row),
|
||||
metadata: isPlainRecord(row.metadata) ? row.metadata : null,
|
||||
}));
|
||||
const missingIds = new Set(await findMissingLocalSkillIds(skills));
|
||||
|
|
@ -2152,11 +2183,23 @@ export function companySkillService(db: Db) {
|
|||
if (skill.sourceType !== "local_path") continue;
|
||||
|
||||
if (!missingIds.has(skill.id)) {
|
||||
if (getMissingSourceMarker(skill.metadata)) {
|
||||
const metadata = getMissingSourceMarker(skill.metadata)
|
||||
? withoutMissingSourceMarker(skill.metadata)
|
||||
: skill.metadata;
|
||||
const sourceLocator = asString(skill.sourceLocator);
|
||||
const nextInventory = sourceLocator
|
||||
? await collectLocalSkillInventory(sourceLocator, inferLocalSkillInventoryMode(skill)).catch(() => null)
|
||||
: null;
|
||||
const nextTrustLevel = nextInventory ? deriveTrustLevel(nextInventory) : skill.trustLevel;
|
||||
const inventoryChanged = nextInventory ? !inventoryEntriesEqual(skill.fileInventory, nextInventory) : false;
|
||||
const metadataChanged = JSON.stringify(metadata ?? {}) !== JSON.stringify(skill.metadata ?? {});
|
||||
if (inventoryChanged || metadataChanged || nextTrustLevel !== skill.trustLevel) {
|
||||
await db
|
||||
.update(companySkills)
|
||||
.set({
|
||||
metadata: withoutMissingSourceMarker(skill.metadata),
|
||||
...(nextInventory ? { fileInventory: serializeFileInventory(nextInventory) } : {}),
|
||||
trustLevel: nextTrustLevel,
|
||||
metadata,
|
||||
updatedAt: new Date(),
|
||||
})
|
||||
.where(eq(companySkills.id, skill.id));
|
||||
|
|
|
|||
|
|
@ -72,6 +72,7 @@ function remoteProviderHttpError(error: unknown, context: {
|
|||
provider: SecretProvider;
|
||||
providerConfigId: string;
|
||||
operation: string;
|
||||
providerConfig?: Record<string, unknown> | null;
|
||||
}): HttpError {
|
||||
if (isSecretProviderClientError(error)) {
|
||||
logger.warn(
|
||||
|
|
@ -85,7 +86,7 @@ function remoteProviderHttpError(error: unknown, context: {
|
|||
},
|
||||
"remote secret provider request failed",
|
||||
);
|
||||
return new HttpError(error.status, error.message, { code: error.code });
|
||||
return new HttpError(error.status, error.message, safeRemoteProviderErrorDetails(error, context));
|
||||
}
|
||||
if (error instanceof HttpError) return error;
|
||||
logger.warn(
|
||||
|
|
@ -99,7 +100,46 @@ function remoteProviderHttpError(error: unknown, context: {
|
|||
},
|
||||
"remote secret provider request failed",
|
||||
);
|
||||
return new HttpError(502, "Remote secret provider request failed.", { code: "provider_error" });
|
||||
return new HttpError(502, "Remote secret provider request failed.", safeRemoteProviderErrorDetails(null, context));
|
||||
}
|
||||
|
||||
function safeRemoteProviderErrorDetails(
|
||||
error: { code: string } | null,
|
||||
context: {
|
||||
provider: SecretProvider;
|
||||
providerConfigId: string;
|
||||
operation: string;
|
||||
providerConfig?: Record<string, unknown> | null;
|
||||
},
|
||||
): Record<string, unknown> {
|
||||
if (
|
||||
context.provider !== "aws_secrets_manager" ||
|
||||
context.operation !== "secret_provider_config.discovery.preview"
|
||||
) {
|
||||
return { code: error?.code ?? "provider_error" };
|
||||
}
|
||||
const details: Record<string, unknown> = {
|
||||
code: error?.code ?? "provider_error",
|
||||
provider: context.provider,
|
||||
operation: context.operation,
|
||||
providerConfigId: context.providerConfigId,
|
||||
};
|
||||
const region = safeString(context.providerConfig?.region);
|
||||
if (region) details.region = region;
|
||||
details.providerVaultContext = context.providerConfigId === "discovery-preview" ? "draft_config" : "provider_config";
|
||||
details.credentialPath = "Paperclip server runtime/provider credential path";
|
||||
if (error?.code === "access_denied") {
|
||||
details.requiredCapability = "secretsmanager:ListSecrets";
|
||||
details.actionableMessage =
|
||||
"AWS discovery preview needs secretsmanager:ListSecrets in the selected region for the Paperclip server runtime/provider credential path.";
|
||||
details.safeAlternative =
|
||||
"If the operator already knows the exact AWS Secrets Manager ARN, paste/link that ARN instead of using discovery. Exact-resource DescribeSecret and runtime read permissions are still required.";
|
||||
}
|
||||
return details;
|
||||
}
|
||||
|
||||
function safeString(value: unknown): string | null {
|
||||
return typeof value === "string" && value.trim() ? value.trim() : null;
|
||||
}
|
||||
|
||||
function remoteImportRowFailureReason(error: unknown, fallback: string, context: {
|
||||
|
|
@ -1091,6 +1131,7 @@ export function secretService(db: Db) {
|
|||
provider: providerId,
|
||||
providerConfigId: "discovery-preview",
|
||||
operation: "secret_provider_config.discovery.preview",
|
||||
providerConfig: parsed.data.config,
|
||||
});
|
||||
}
|
||||
},
|
||||
|
|
|
|||
|
|
@ -120,6 +120,20 @@ describe("writeFileViewerStateToSearch", () => {
|
|||
expect(params.get("keep")).toBe("yes");
|
||||
});
|
||||
|
||||
it("clears browse-origin viewer params when closing a selected file", () => {
|
||||
const next = writeFileViewerStateToSearch(
|
||||
"?tab=thread&browse=1&q=FileViewer&folder=ui/src&file=ui/src/FileViewer.tsx&line=4",
|
||||
null,
|
||||
);
|
||||
const params = new URLSearchParams(next);
|
||||
expect(params.get("file")).toBeNull();
|
||||
expect(params.get("line")).toBeNull();
|
||||
expect(params.get("browse")).toBeNull();
|
||||
expect(params.get("q")).toBeNull();
|
||||
expect(params.get("folder")).toBeNull();
|
||||
expect(params.get("tab")).toBe("thread");
|
||||
});
|
||||
|
||||
it("returns empty string when no params remain", () => {
|
||||
const next = writeFileViewerStateToSearch("?file=a.ts", null);
|
||||
expect(next).toBe("");
|
||||
|
|
|
|||
|
|
@ -285,7 +285,10 @@ function EnabledFileViewerProvider({ issueId, children }: Omit<FileViewerProvide
|
|||
}, [location.search, navigateSearch]);
|
||||
|
||||
const close = useCallback(() => {
|
||||
const params = new URLSearchParams(writeFileViewerStateToSearch(location.search, null).replace(/^\?/, ""));
|
||||
const currentSearch = typeof window === "undefined"
|
||||
? location.search
|
||||
: (window.location.search || location.search);
|
||||
const params = new URLSearchParams(writeFileViewerStateToSearch(currentSearch, null).replace(/^\?/, ""));
|
||||
params.delete("browse");
|
||||
params.delete("q");
|
||||
params.delete("folder");
|
||||
|
|
|
|||
|
|
@ -4525,6 +4525,13 @@ function IssueFileViewer({
|
|||
const viewer = useRequiredFileViewer();
|
||||
const open = viewer.state !== null || viewer.browse || promptOpen;
|
||||
const showPromptWhenEmpty = (promptOpen || viewer.browse) && viewer.state === null;
|
||||
|
||||
useEffect(() => {
|
||||
if (!promptOpen) return;
|
||||
if (viewer.state === null && !viewer.browse) return;
|
||||
onPromptOpenChange(false);
|
||||
}, [onPromptOpenChange, promptOpen, viewer.browse, viewer.state]);
|
||||
|
||||
return (
|
||||
<FileViewerSheet
|
||||
issueId={issueId}
|
||||
|
|
|
|||
|
|
@ -1,7 +1,7 @@
|
|||
// @vitest-environment jsdom
|
||||
|
||||
import { act } from "react";
|
||||
import { createRoot } from "react-dom/client";
|
||||
import { flushSync } from "react-dom";
|
||||
import { MemoryRouter } from "react-router-dom";
|
||||
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||
import type {
|
||||
|
|
@ -133,6 +133,14 @@ const providerConfigs = [
|
|||
},
|
||||
] satisfies Partial<CompanySecretProviderConfig>[];
|
||||
|
||||
async function act(callback: () => void | Promise<void>) {
|
||||
let result: void | Promise<void> = undefined;
|
||||
flushSync(() => {
|
||||
result = callback();
|
||||
});
|
||||
await result;
|
||||
}
|
||||
|
||||
async function flushReact() {
|
||||
await act(async () => {
|
||||
await Promise.resolve();
|
||||
|
|
@ -564,9 +572,24 @@ describe("Secrets page layout", () => {
|
|||
});
|
||||
|
||||
it("shows AWS discovery errors without replacing manual vault form values", async () => {
|
||||
const rawProviderMessage =
|
||||
"AccessDeniedException: User: arn:aws:sts::123456789012:assumed-role/prod/Paperclip is not authorized";
|
||||
mockSecretsApi.providerConfigDiscoveryPreview.mockRejectedValueOnce(
|
||||
new ApiError("AWS Secrets Manager denied the request. Check IAM permissions for this provider vault.", 403, {
|
||||
details: { code: "access_denied" },
|
||||
details: {
|
||||
code: "access_denied",
|
||||
provider: "aws_secrets_manager",
|
||||
operation: "secret_provider_config.discovery.preview",
|
||||
providerConfigId: "discovery-preview",
|
||||
providerVaultContext: "draft_config",
|
||||
region: "us-west-2",
|
||||
credentialPath: "Paperclip server runtime/provider credential path",
|
||||
requiredCapability: "secretsmanager:ListSecrets",
|
||||
actionableMessage:
|
||||
"AWS discovery preview needs secretsmanager:ListSecrets in the selected region for the Paperclip server runtime/provider credential path.",
|
||||
safeAlternative:
|
||||
"If the operator already knows the exact AWS Secrets Manager ARN, paste/link that ARN instead of using discovery. Exact-resource DescribeSecret and runtime read permissions are still required.",
|
||||
},
|
||||
}),
|
||||
);
|
||||
const root = createRoot(container);
|
||||
|
|
@ -604,7 +627,20 @@ describe("Secrets page layout", () => {
|
|||
await flushReact();
|
||||
await flushReact();
|
||||
|
||||
expect(document.body.textContent).toContain("AWS Secrets Manager denied the request");
|
||||
const errorBanner = document.querySelector('[data-testid="aws-vault-discovery-error"]');
|
||||
expect(errorBanner).not.toBeNull();
|
||||
expect(errorBanner?.textContent).toContain("AWS discovery needs ListSecrets permission");
|
||||
expect(errorBanner?.textContent).toContain("secretsmanager:ListSecrets");
|
||||
expect(errorBanner?.textContent).toContain("Paperclip server runtime/provider credential path");
|
||||
expect(errorBanner?.textContent).toContain("paste/link that ARN");
|
||||
expect(errorBanner?.textContent).toContain("DescribeSecret");
|
||||
expect(errorBanner?.textContent).toContain("us-west-2");
|
||||
expect(errorBanner?.textContent).toContain("secret_provider_config.discovery.preview");
|
||||
expect(errorBanner?.textContent).toContain("aws_secrets_manager");
|
||||
expect(errorBanner?.textContent).toContain("Safe request/error details");
|
||||
expect(errorBanner?.textContent).not.toContain(rawProviderMessage);
|
||||
expect(errorBanner?.textContent).not.toContain("arn:aws");
|
||||
expect(errorBanner?.textContent).not.toContain("123456789012");
|
||||
expect(regionInput.value).toBe("us-west-2");
|
||||
expect(namespaceInput.value).toBe("manual-prod");
|
||||
|
||||
|
|
@ -613,6 +649,59 @@ describe("Secrets page layout", () => {
|
|||
});
|
||||
});
|
||||
|
||||
it("keeps generic AWS discovery 403 errors on the generic failure path", async () => {
|
||||
mockSecretsApi.providerConfigDiscoveryPreview.mockRejectedValueOnce(
|
||||
new ApiError("AWS discovery request failed before IAM evaluation.", 403, {
|
||||
details: {
|
||||
code: "proxy_forbidden",
|
||||
provider: "aws_secrets_manager",
|
||||
operation: "secret_provider_config.discovery.preview",
|
||||
region: "us-west-1",
|
||||
},
|
||||
}),
|
||||
);
|
||||
const root = createRoot(container);
|
||||
const queryClient = new QueryClient({
|
||||
defaultOptions: { queries: { retry: false } },
|
||||
});
|
||||
|
||||
await act(async () => {
|
||||
root.render(
|
||||
<MemoryRouter>
|
||||
<QueryClientProvider client={queryClient}>
|
||||
<Secrets />
|
||||
</QueryClientProvider>
|
||||
</MemoryRouter>,
|
||||
);
|
||||
});
|
||||
await flushReact();
|
||||
await flushReact();
|
||||
await openAwsVaultDialog();
|
||||
|
||||
const regionInput = document.getElementById("provider-vault-aws-region") as HTMLInputElement;
|
||||
await act(async () => {
|
||||
setInputValue(regionInput, "us-west-1");
|
||||
});
|
||||
await flushReact();
|
||||
|
||||
await act(async () => {
|
||||
(document.querySelector('[data-testid="aws-vault-discovery-button"]') as HTMLButtonElement | null)?.click();
|
||||
});
|
||||
await flushReact();
|
||||
await flushReact();
|
||||
|
||||
const errorBanner = document.querySelector('[data-testid="aws-vault-discovery-error"]');
|
||||
expect(errorBanner).not.toBeNull();
|
||||
expect(errorBanner?.textContent).toContain("AWS discovery failed");
|
||||
expect(errorBanner?.textContent).toContain("AWS discovery request failed before IAM evaluation.");
|
||||
expect(errorBanner?.textContent).toContain("proxy_forbidden");
|
||||
expect(errorBanner?.textContent).not.toContain("AWS discovery needs ListSecrets permission");
|
||||
|
||||
await act(async () => {
|
||||
root.unmount();
|
||||
});
|
||||
});
|
||||
|
||||
it("shows an empty AWS discovery result without blocking manual entry", async () => {
|
||||
mockSecretsApi.providerConfigDiscoveryPreview.mockResolvedValueOnce(
|
||||
makeDiscoveryPreview({ candidates: [], sampledSecretCount: 0 }),
|
||||
|
|
|
|||
|
|
@ -99,6 +99,19 @@ type ProviderVaultForm = {
|
|||
secretPathPrefix: string;
|
||||
};
|
||||
|
||||
type SafeProviderErrorDetails = {
|
||||
code?: string;
|
||||
provider?: string;
|
||||
operation?: string;
|
||||
providerConfigId?: string;
|
||||
providerVaultContext?: string;
|
||||
region?: string;
|
||||
credentialPath?: string;
|
||||
requiredCapability?: string;
|
||||
actionableMessage?: string;
|
||||
safeAlternative?: string;
|
||||
};
|
||||
|
||||
const PROVIDER_ORDER: SecretProvider[] = [
|
||||
"local_encrypted",
|
||||
"aws_secrets_manager",
|
||||
|
|
@ -137,6 +150,34 @@ function providerConfigValue(config: CompanySecretProviderConfig["config"], key:
|
|||
return typeof value === "string" ? value : "";
|
||||
}
|
||||
|
||||
function apiErrorDetails(error: unknown): SafeProviderErrorDetails | null {
|
||||
if (!(error instanceof ApiError)) return null;
|
||||
const body = error.body;
|
||||
if (!body || typeof body !== "object") return null;
|
||||
const details = (body as Record<string, unknown>).details;
|
||||
if (!details || typeof details !== "object" || Array.isArray(details)) return null;
|
||||
return details as SafeProviderErrorDetails;
|
||||
}
|
||||
|
||||
function apiErrorCode(error: unknown): string | null {
|
||||
return apiErrorDetails(error)?.code ?? null;
|
||||
}
|
||||
|
||||
function isAwsDiscoveryAccessDenied(error: unknown): boolean {
|
||||
const details = apiErrorDetails(error);
|
||||
if (details?.provider === "aws_secrets_manager" && details.operation === "secret_provider_config.discovery.preview") {
|
||||
return details.code === "access_denied";
|
||||
}
|
||||
if (!(error instanceof ApiError)) return false;
|
||||
return apiErrorCode(error) === "access_denied";
|
||||
}
|
||||
|
||||
function readableErrorMessage(error: unknown): string {
|
||||
if (error instanceof ApiError) return error.message || `Request failed: ${error.status}`;
|
||||
if (error instanceof Error) return error.message;
|
||||
return "Unexpected error";
|
||||
}
|
||||
|
||||
function providerVaultFormFromConfig(config: CompanySecretProviderConfig): ProviderVaultForm {
|
||||
return {
|
||||
...emptyProviderVaultForm(config.provider),
|
||||
|
|
@ -389,7 +430,7 @@ export function Secrets() {
|
|||
const [vaultError, setVaultError] = useState<string | null>(null);
|
||||
const [vaultDiscovery, setVaultDiscovery] =
|
||||
useState<SecretProviderConfigDiscoveryPreviewResult | null>(null);
|
||||
const [vaultDiscoveryError, setVaultDiscoveryError] = useState<string | null>(null);
|
||||
const [vaultDiscoveryError, setVaultDiscoveryError] = useState<unknown | null>(null);
|
||||
|
||||
useEffect(() => {
|
||||
setBreadcrumbs([{ label: "Secrets" }]);
|
||||
|
|
@ -678,7 +719,7 @@ export function Secrets() {
|
|||
},
|
||||
onError: (error) => {
|
||||
setVaultDiscovery(null);
|
||||
setVaultDiscoveryError(error instanceof ApiError ? error.message : (error as Error).message);
|
||||
setVaultDiscoveryError(error);
|
||||
},
|
||||
});
|
||||
|
||||
|
|
@ -2145,7 +2186,7 @@ function AwsProviderVaultDiscoveryPanel({
|
|||
}: {
|
||||
form: ProviderVaultForm;
|
||||
preview: SecretProviderConfigDiscoveryPreviewResult | null;
|
||||
error: string | null;
|
||||
error: unknown | null;
|
||||
loading: boolean;
|
||||
onDiscover: () => void;
|
||||
onApply: (candidate: SecretProviderConfigDiscoveryCandidate) => void;
|
||||
|
|
@ -2191,13 +2232,7 @@ function AwsProviderVaultDiscoveryPanel({
|
|||
) : null}
|
||||
|
||||
{error ? (
|
||||
<div
|
||||
className="flex items-start gap-2 rounded-md border border-destructive/30 bg-destructive/5 p-3 text-xs text-destructive"
|
||||
role="alert"
|
||||
>
|
||||
<AlertCircle className="mt-0.5 h-3.5 w-3.5 shrink-0" />
|
||||
<span>{error}</span>
|
||||
</div>
|
||||
<AwsProviderVaultDiscoveryError form={form} error={error} />
|
||||
) : null}
|
||||
|
||||
{warnings.length > 0 ? (
|
||||
|
|
@ -2241,6 +2276,96 @@ function AwsProviderVaultDiscoveryPanel({
|
|||
);
|
||||
}
|
||||
|
||||
function AwsProviderVaultDiscoveryError({
|
||||
form,
|
||||
error,
|
||||
}: {
|
||||
form: ProviderVaultForm;
|
||||
error: unknown;
|
||||
}) {
|
||||
const details = apiErrorDetails(error);
|
||||
const isAccessDenied = isAwsDiscoveryAccessDenied(error);
|
||||
const region = (details?.region ?? form.region.trim()) || "unspecified";
|
||||
const message = readableErrorMessage(error);
|
||||
const safeDetails = {
|
||||
message,
|
||||
status: error instanceof ApiError ? error.status : undefined,
|
||||
provider: details?.provider ?? form.provider,
|
||||
operation: details?.operation ?? "secret_provider_config.discovery.preview",
|
||||
providerVaultContext: details?.providerVaultContext ?? "draft_config",
|
||||
region,
|
||||
code: details?.code,
|
||||
requiredCapability: details?.requiredCapability,
|
||||
credentialPath: details?.credentialPath,
|
||||
safeAlternative: details?.safeAlternative,
|
||||
};
|
||||
const detailsText = JSON.stringify(safeDetails, null, 2);
|
||||
|
||||
const copyDetails = () => {
|
||||
void navigator.clipboard?.writeText(detailsText);
|
||||
};
|
||||
|
||||
return (
|
||||
<div
|
||||
className="rounded-md border border-destructive/30 bg-destructive/5 p-3 text-xs text-destructive"
|
||||
role="alert"
|
||||
data-testid="aws-vault-discovery-error"
|
||||
>
|
||||
<div className="flex items-start gap-2">
|
||||
<AlertCircle className="mt-0.5 h-3.5 w-3.5 shrink-0" />
|
||||
<div className="min-w-0 flex-1 space-y-2">
|
||||
<div>
|
||||
<p className="font-medium">
|
||||
{isAccessDenied ? "AWS discovery needs ListSecrets permission" : "AWS discovery failed"}
|
||||
</p>
|
||||
<p className="mt-1 leading-relaxed text-destructive/85">
|
||||
{isAccessDenied
|
||||
? details?.actionableMessage ??
|
||||
"Discovery needs secretsmanager:ListSecrets in the selected region for the Paperclip server runtime/provider credential path."
|
||||
: message}
|
||||
</p>
|
||||
</div>
|
||||
{isAccessDenied ? (
|
||||
<p className="leading-relaxed text-destructive/85">
|
||||
{details?.safeAlternative ??
|
||||
"If you already know the exact AWS Secrets Manager ARN, paste/link that ARN instead of using discovery. Exact-resource DescribeSecret and runtime read permissions are still required."}
|
||||
</p>
|
||||
) : null}
|
||||
<dl className="grid gap-1 text-destructive/80 sm:grid-cols-2">
|
||||
<div>
|
||||
<dt className="font-medium">Region</dt>
|
||||
<dd>{region}</dd>
|
||||
</div>
|
||||
<div>
|
||||
<dt className="font-medium">Operation</dt>
|
||||
<dd>{details?.operation ?? "secret_provider_config.discovery.preview"}</dd>
|
||||
</div>
|
||||
<div>
|
||||
<dt className="font-medium">Provider</dt>
|
||||
<dd>{details?.provider ?? "aws_secrets_manager"}</dd>
|
||||
</div>
|
||||
<div>
|
||||
<dt className="font-medium">Vault context</dt>
|
||||
<dd>{details?.providerVaultContext ?? "draft_config"}</dd>
|
||||
</div>
|
||||
</dl>
|
||||
<div className="rounded-md border border-destructive/20 bg-background/70 p-2 text-foreground">
|
||||
<div className="mb-1 flex items-center justify-between gap-2">
|
||||
<span className="font-medium text-muted-foreground">Safe request/error details</span>
|
||||
<Button type="button" variant="ghost" size="sm" onClick={copyDetails}>
|
||||
Copy
|
||||
</Button>
|
||||
</div>
|
||||
<pre className="max-h-36 overflow-auto whitespace-pre-wrap break-words font-mono text-[11px] leading-relaxed">
|
||||
{detailsText}
|
||||
</pre>
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
function AwsProviderVaultDiscoveryCandidateRow({
|
||||
candidate,
|
||||
onApply,
|
||||
|
|
|
|||
Loading…
Reference in New Issue