fix(review): address review findings (guardrails, prompts, tests, docs)
- permission-gate: block refs/tags/v* pushes, rm -r -f separated flags, git reset --hard, git clean (verified against actual bypasses) - prompts: /bugfix /feature /review no longer hardcode go-standards — reference profile-gated <lang>-standards instead - company-context: drop hardcoded Go stack, note TRACKER_URL priority, warn on context truncation instead of silently dropping rules - repo-map/docs-map: graceful degradation when config values are TODO - /kit-doctor: warn on unfilled config fields (repoMap/trackerUrl/docsUrl) - audit: retry POSTs to endpoint (3 attempts, backoff), still best-effort - install.sh: remove TODO course URL from cheat sheet - tests: expand guardrails (43 node checks), add shell tests for create-mr.sh (scp/https origin parse, GITLAB_HOST override, protected branch refusal), cover company-context lib (normalize/fetch/truncation) and mcp-bridge - commit package-lock.json for reproducible installs - document npm test Node >= 22.6 requirement (type stripping)
This commit is contained in:
Aleksey Shakhmatov
+110
-4
@@ -5,8 +5,18 @@
|
||||
*
|
||||
* Extensions only `import type` the Pi API, so they run here with node's
|
||||
* type-stripping and mocked pi/ctx objects.
|
||||
*
|
||||
* NOTE: type stripping requires Node.js >= 22.6 (install.sh's >= 20 applies to
|
||||
* employees installing pi, not to this test harness).
|
||||
*/
|
||||
|
||||
const [NODE_MAJOR, NODE_MINOR] = process.versions.node.split(".").map(Number);
|
||||
if (NODE_MAJOR < 22 || (NODE_MAJOR === 22 && NODE_MINOR < 6)) {
|
||||
console.error(`npm test требует Node.js >= 22.6 (type stripping), у тебя ${process.version}.`);
|
||||
console.error("Это ограничение тест-харнеса, а не install.sh (для сотрудников достаточно >= 20).");
|
||||
process.exit(1);
|
||||
}
|
||||
|
||||
process.env.PI_KIT_AUDIT_OFF = "1"; // don't touch the real audit log during tests
|
||||
|
||||
import protectedPaths from "../extensions/protected-paths.ts";
|
||||
@@ -14,7 +24,10 @@ import permissionGate from "../extensions/permission-gate.ts";
|
||||
import secretScanner from "../extensions/secret-scanner.ts";
|
||||
import commitGuard from "../extensions/commit-guard.ts";
|
||||
import llmRedaction from "../extensions/llm-redaction.ts";
|
||||
import companyContext from "../extensions/company-context.ts";
|
||||
import mcpBridge from "../extensions/mcp-bridge.ts";
|
||||
import { redact, scanSecrets } from "../extensions/lib/secrets.ts";
|
||||
import { normalize, renderContext, fetchRemoteConfig, MAX_CONTEXT_LINES } from "../extensions/lib/company-config.ts";
|
||||
|
||||
let fails = 0;
|
||||
function check(name: string, cond: boolean) {
|
||||
@@ -29,6 +42,13 @@ function grab(ext: (pi: any) => void, evName: string): any {
|
||||
return handler;
|
||||
}
|
||||
|
||||
/** Collect registerCommand calls (for extensions that register commands). */
|
||||
function grabCommands(ext: (pi: any) => void): string[] {
|
||||
const names: string[] = [];
|
||||
ext({ on() {}, registerCommand: (name: string) => names.push(name) });
|
||||
return names;
|
||||
}
|
||||
|
||||
const noUI: any = { hasUI: false };
|
||||
const AKIA = "AKIA" + "ABCDEFGHIJKLMNOP";
|
||||
const GLPAT = "glpat-" + "aBcDeF1234567890ghXY12"; // 22 chars after prefix (>=20)
|
||||
@@ -40,12 +60,35 @@ async function run() {
|
||||
// --- protected-paths ---
|
||||
const pp = grab(protectedPaths, "tool_call");
|
||||
check("protected: block .env", !!(await pp({ toolName: "write", input: { path: ".env" } }, noUI))?.block);
|
||||
check("protected: block secrets/ file", !!(await pp({ toolName: "write", input: { path: "deploy/secrets/db.sh" } }, noUI))?.block);
|
||||
check("protected: block .git internals", !!(await pp({ toolName: "edit", input: { path: ".git/config" } }, noUI))?.block);
|
||||
check("protected: allow src/app.go", !(await pp({ toolName: "write", input: { path: "src/app.go" } }, noUI))?.block);
|
||||
|
||||
// --- permission-gate ---
|
||||
// --- permission-gate (table: command -> should block?) ---
|
||||
const pg = grab(permissionGate, "tool_call");
|
||||
check("perm: block push v* tag", !!(await pg({ toolName: "bash", input: { command: "git push origin v1.2.3" } }, noUI))?.block);
|
||||
check("perm: allow ls", !(await pg({ toolName: "bash", input: { command: "ls -la" } }, noUI))?.block);
|
||||
const permCases: Array<[string, string, boolean]> = [
|
||||
["perm: block push v1.2.3 tag", "git push origin v1.2.3", true],
|
||||
["perm: block push tags/v1.2.3", "git push origin tags/v1.2.3", true],
|
||||
["perm: block push refs/tags/v1.2.3", "git push origin refs/tags/v1.2.3", true],
|
||||
["perm: block push main", "git push origin main", true],
|
||||
["perm: block push HEAD:refs/heads/main", "git push origin HEAD:refs/heads/main", true],
|
||||
["perm: block push release branch", "git push origin release-42", true],
|
||||
["perm: block force push", "git push --force origin feature/x", true],
|
||||
["perm: block push --tags", "git push origin --tags", true],
|
||||
["perm: block rm -rf", "rm -rf /tmp/x", true],
|
||||
["perm: block rm -r -f separated", "rm -r -f /tmp/x", true],
|
||||
["perm: block rm -f -r separated", "rm -f -r -- /tmp/x", true],
|
||||
["perm: block rm -fr", "rm -fr /tmp/x", true],
|
||||
["perm: block git reset --hard", "git reset --hard HEAD~1", true],
|
||||
["perm: block git clean -fdx", "git clean -fdx", true],
|
||||
["perm: allow rm -i single", "rm -i /tmp/notes.txt", false],
|
||||
["perm: allow ls", "ls -la", false],
|
||||
["perm: allow push feature branch", "git push origin feature/PROJ-1", false],
|
||||
];
|
||||
for (const [name, cmd, blocked] of permCases) {
|
||||
const res = await pg({ toolName: "bash", input: { command: cmd } }, noUI);
|
||||
check(name, blocked ? !!res?.block : !res?.block);
|
||||
}
|
||||
|
||||
// --- secret-scanner ---
|
||||
const ss = grab(secretScanner, "tool_call");
|
||||
@@ -68,11 +111,74 @@ async function run() {
|
||||
const redacted = JSON.stringify(out?.messages ?? []);
|
||||
check("redact: token removed from payload", !redacted.includes(GLPAT) && redacted.includes("REDACTED"));
|
||||
check("redact: no-op when clean", (await lr({ payload: { messages: [{ role: "user", content: "hello" }] } })) === undefined);
|
||||
{
|
||||
const deep = { messages: [{ content: [{ type: "text", text: `k ${AKIA}` }] }] };
|
||||
const r: any = await lr({ payload: deep });
|
||||
check("redact: deep object arrays", !!r && !JSON.stringify(r.messages).includes(AKIA));
|
||||
}
|
||||
|
||||
// --- lib units ---
|
||||
// --- lib units: secrets ---
|
||||
check("scanSecrets finds jwt", scanSecrets(JWT).length > 0);
|
||||
check("redact keeps clean text", redact("just normal text") === "just normal text");
|
||||
|
||||
// --- lib units: company-config ---
|
||||
check("normalize: junk input falls back to defaults", normalize({ foo: 1, gitHost: 42, rules: "nope" }).gitHost === "TODO");
|
||||
check("normalize: drops non-string rules", JSON.stringify(normalize({ rules: [1, "keep", null] }).rules) === '["keep"]');
|
||||
check("normalize: keeps known fields", normalize({ gitHost: "h", trackerUrl: "t", docsUrl: "d", repoMap: "m" }).gitHost === "h");
|
||||
check("renderContext: replaces placeholders", renderContext("host={{gitHost}}", normalize({ gitHost: "gh" })).text === "host=gh");
|
||||
check("renderContext: rules block", renderContext("{{rules}}", normalize({ rules: ["a", "b"] })).text.includes("- a\n- b"));
|
||||
check("renderContext: no truncation for short template", !renderContext("# t\n{{rules}}", normalize({})).truncated);
|
||||
{
|
||||
const longTemplate = Array.from({ length: MAX_CONTEXT_LINES + 10 }, (_, i) => `line ${i}`).join("\n");
|
||||
const r = renderContext(longTemplate, normalize({}));
|
||||
check("renderContext: truncates long template", r.truncated && r.text.split("\n").length === MAX_CONTEXT_LINES);
|
||||
}
|
||||
{
|
||||
const okFetch: any = async () => ({ ok: true, json: async () => ({ gitHost: "remote", rules: [1, "a"] }) });
|
||||
const remote = await fetchRemoteConfig("https://x", okFetch);
|
||||
check("fetchRemoteConfig: success normalizes", remote?.gitHost === "remote" && JSON.stringify(remote.rules) === '["a"]');
|
||||
}
|
||||
{
|
||||
const badFetch: any = async () => ({ ok: false });
|
||||
check("fetchRemoteConfig: !ok returns null", (await fetchRemoteConfig("https://x", badFetch)) === null);
|
||||
}
|
||||
{
|
||||
const throwFetch: any = async () => { throw new Error("net"); };
|
||||
check("fetchRemoteConfig: throw returns null", (await fetchRemoteConfig("https://x", throwFetch)) === null);
|
||||
}
|
||||
|
||||
// --- company-context extension (no remote configured) ---
|
||||
// Must use ONE extension instance: the rendered context is cached per-instance.
|
||||
{
|
||||
let sessionHandler: any;
|
||||
let beforeAgentHandler: any;
|
||||
companyContext({
|
||||
on(ev: string, fn: any) {
|
||||
if (ev === "session_start") sessionHandler = fn;
|
||||
if (ev === "before_agent_start") beforeAgentHandler = fn;
|
||||
},
|
||||
registerCommand() {},
|
||||
} as any);
|
||||
await sessionHandler({ reason: "test" }, noUI);
|
||||
const inj = await beforeAgentHandler({ systemPrompt: "base" });
|
||||
check("company-context: injects corporate block", typeof inj?.systemPrompt === "string" && inj.systemPrompt.includes("Корпоративный контекст"));
|
||||
check("company-context: preserves base prompt", inj?.systemPrompt.startsWith("base"));
|
||||
check("company-context: registers /kit-config", grabCommands(companyContext).includes("kit-config"));
|
||||
}
|
||||
|
||||
// --- mcp-bridge: disabled servers must not connect or error ---
|
||||
{
|
||||
const registered: string[] = [];
|
||||
const tools: string[] = [];
|
||||
await mcpBridge({
|
||||
registerCommand: (name: string) => registered.push(name),
|
||||
registerTool: () => tools.push("x"),
|
||||
on() {},
|
||||
} as any);
|
||||
check("mcp-bridge: registers /mcp-status", registered.includes("mcp-status"));
|
||||
check("mcp-bridge: no tools with all servers disabled", tools.length === 0);
|
||||
}
|
||||
|
||||
console.log(fails === 0 ? "\nALL PASS" : `\n${fails} FAILED`);
|
||||
process.exit(fails === 0 ? 0 : 1);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,60 @@
|
||||
#!/usr/bin/env bash
|
||||
# Shell smoke tests for skills/jira-workflow/scripts/create-mr.sh.
|
||||
# Network-free: uses MR_DRY_RUN=1 and a throwaway git repo.
|
||||
# Run via `npm test` (after guardrails.test.ts) or directly: bash test/shell.test.sh
|
||||
set -euo pipefail
|
||||
|
||||
ROOT="$(cd "$(dirname "$0")/.." && pwd)"
|
||||
SCRIPT="$ROOT/skills/jira-workflow/scripts/create-mr.sh"
|
||||
FAILS=0
|
||||
|
||||
check() { # <name> <expected-substring> <actual>
|
||||
local name="$1" expected="$2" actual="$3"
|
||||
if grep -qF -- "$expected" <<<"$actual"; then
|
||||
echo "PASS $name"
|
||||
else
|
||||
echo "FAIL $name (expected substring: $expected)"
|
||||
echo "--- actual ---"
|
||||
echo "$actual"
|
||||
FAILS=$((FAILS + 1))
|
||||
fi
|
||||
}
|
||||
|
||||
# Fresh throwaway repo with an empty commit.
|
||||
DIR="$(mktemp -d)"
|
||||
trap 'rm -rf "$DIR"' EXIT
|
||||
cd "$DIR"
|
||||
git init -q
|
||||
git config user.email test@example.com
|
||||
git config user.name Test
|
||||
git remote add origin git@gitlab.tech.mvideo.ru:ai/foo.git
|
||||
git commit --allow-empty -q -m "chore: init"
|
||||
git checkout -q -b feature/PROJ-1-smoke
|
||||
|
||||
# 1. scp-like origin → API URL with encoded namespace.
|
||||
OUT="$(GITLAB_TOKEN=dummy MR_DRY_RUN=1 bash "$SCRIPT" "Test MR" 2>&1)"
|
||||
check "create-mr: scp-like origin parse" "POST https://gitlab.tech.mvideo.ru/api/v4/projects/ai%2Ffoo/merge_requests" "$OUT"
|
||||
check "create-mr: dry-run shows would-push" "would push: git push -u origin feature/PROJ-1-smoke" "$OUT"
|
||||
|
||||
# 2. https-like origin → same API shape, different host.
|
||||
git remote set-url origin "https://git.codelab.vc/ai/bar.git"
|
||||
OUT="$(GITLAB_TOKEN=dummy MR_DRY_RUN=1 bash "$SCRIPT" "Test MR" 2>&1)"
|
||||
check "create-mr: https origin parse" "POST https://git.codelab.vc/api/v4/projects/ai%2Fbar/merge_requests" "$OUT"
|
||||
|
||||
# 3. GITLAB_HOST overrides the derived host.
|
||||
OUT="$(GITLAB_TOKEN=dummy GITLAB_HOST=gitlab.example.com MR_DRY_RUN=1 bash "$SCRIPT" "Test MR" 2>&1)"
|
||||
check "create-mr: GITLAB_HOST override" "POST https://gitlab.example.com/api/v4/projects/ai%2Fbar/merge_requests" "$OUT"
|
||||
|
||||
# 4. Refuses to create MR from a protected/release source branch.
|
||||
git checkout -q -b release-42
|
||||
OUT="$(GITLAB_TOKEN=dummy MR_DRY_RUN=1 bash "$SCRIPT" "Test MR" 2>&1 || true)"
|
||||
check "create-mr: refuses release-* source" "Refusing: source branch" "$OUT"
|
||||
|
||||
echo ""
|
||||
if [ "$FAILS" -eq 0 ]; then
|
||||
echo "shell tests: ALL PASS"
|
||||
exit 0
|
||||
else
|
||||
echo "shell tests: $FAILS FAILED"
|
||||
exit 1
|
||||
fi
|
||||
Reference in New Issue
Block a user