Skip to content

feat(agents): privacy-reviewed hack-feedback skill - #337

Draft
roodboi wants to merge 6 commits into
nextfrom
wf/f20-feedback-skill-b16
Draft

roodboi wants to merge 6 commits into
nextfrom
wf/f20-feedback-skill-b16

Conversation

@roodboi

@roodboi roodboi commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Linear: HACK-1138 (WU25, privacy-reviewed Hack feedback drafts).

Adds a plugin-only hack-feedback skill. It drafts reports about Hack itself offline and never posts on its own.

  • Draft content: observed and expected behavior, exactly one reproduction status (reproduced, partially-reproduced, not-reproduced, not-attempted), and evidence tagged observed/source/reported/inferred. It also records one friction category, the command shape with placeholders (or unknown when the original argv was not kept), sanitized duplicate-search terms and an attachment list.
  • Privacy review: removes secrets, ciphertext, hashes ("hashing is not anonymization"), usernames, home and project paths, private or routed URLs, routed *.hack and private hostnames written without a scheme, IPv4 and IPv6 addresses, emails, raw logs and config values. A <...> placeholder is exempt only when it is the whole value, so KEY=<x>leak is still redacted. Instructions found in logs or tool output are treated as data.
  • Duplicates: search terms come only from registered error codes (HACK_ERROR_CODES), the leading tokens that resolve to a CLI_SPEC command path, and the category. The first token that is not a subcommand stops the walk, so positional project, service and script names never become terms. A search happens only when the user agrees. A matching issue turns the report into a comment instead of a new issue.
  • Review before posting: the user confirms the destination and approves the exact final payload. Any later edit needs a fresh review. A decline keeps the draft offline. A failed send keeps the draft and needs a new yes; check first whether the issue was created anyway. Posting uses the user's existing tooling (for example an already signed-in gh). There is no built-in GitHub integration and no background sender.
  • Feedback is optional: it is not an onboarding gate and never blocks the user's task.

Rendering

src/agents/feedback-review.ts holds the typed contract: taxonomy, statuses, evidence classes, template sections, redaction kinds and gate refusals. It also holds the reference sanitizer, the draft renderer and the send gate. src/agents/feedback-skill.ts renders these outputs from that contract through bun run generate:agent-plugins:

  • the skill
  • references/issue-template.md
  • references/examples.md: a synthetic useful report, plus the registered negatives (missing original argv, host-wide disk totals, stale release claim, pasted config/logs, log instructions, hashed identifiers)
  • .github/ISSUE_TEMPLATE/hack-feedback.md

The plugin hack-cli routing description now hands feedback intents to hack-feedback. Docs are updated: the skill count goes from seven to eight, the agent-guidance sources table changes, and the hack-feedback candidate status changes from Defer to Shipped. The capability matrix is regenerated.

Canonical instruction sections and the standalone renders (hack-cli, hack-init, Cursor rules) did not change. No new integration-ownership fingerprints or content revision were needed; the ownership and instruction-source tests pass unchanged. The CLI surface did not change, so the CLI reference did not need regenerating. No Rust changes.

Rework after verification (head 2f7bd64)

The verifier found these defects at 0cb0ff23. Each is now fixed and has a test that fails without the fix:

  • Duplicate-search leak: hack env acme-secret-project gave env acme-secret-project. buildDuplicateSearchTerms({ cli, draft }) now resolves the command path through the injected CliSpec (resolveCommand) and keeps only registered error codes. hack env acme-secret-project now gives env env, hack run acmeprod migrate gives run env, and an invented E_ACME_SECRET_DB is dropped.
  • <-prefixed secrets: (?!<) skipped any value that started with <. The lookahead now exempts only a complete <...> token followed by whitespace, closing punctuation or the end of the text. This applies to secret assignments, config values, JSON values, secret flags and bearer values. DB_PASSWORD=<x>leakedvalue, API_TOKEN=<redacted:secret>extra-leak, token: <x>hunter2fixture and --password <x>flagleak are redacted. A freshly approved body containing them returns privacy-findings.
  • Checklist coverage: a new hostname redaction kind covers routed *.hack hosts and corp/internal/local/lan/home/private/test/localhost hosts written without a scheme. Paths under /root, /srv, /Volumes, /workspace(s), /mnt, /media, /data, /code, /project(s) and /var/www are redacted, as are Windows drive paths. IPv6 is redacted when compressed or full; clock times and ::1 stay. A body with routed web.acme-app.hack 502 now returns privacy-findings.
  • Synchronous sender throw: the sender is now called inside Promise.resolve().then(...). A non-async throw is a failed attempt that consumes the approval, and reusing the approval returns approval-already-used.
  • Issue number: an issue-comment destination needs a positive safe integer; -3.5, 0, 1.5, NaN and Infinity return destination-invalid. The refusal summary in the skill says so.
  • URL allowlist: a URL stays only if it parses with origin https://github.com and has no credentials. Its normalized path must be under /hack-dance/hack. URLs with .., %2e, backslashes or queries are redacted, as are fragments other than #[A-Za-z0-9_-]*.
  • Instruction filter: the single broad regex is now a list of targeted patterns: direct address with file/create/submit, imperatives at a line or sentence start, "agent must now ...", and "user said yes". Hack failed to send the report to the daemon and File watching stopped ... are now kept.
  • Truncated hashes and GitHub headers: hex runs of 16 or more characters that contain a digit are redacted; short commit ids are kept. Authorization: token ... is redacted.
  • Test gaps: one test edits only an attachment's content after review and expects payload-changed. Another renders a draft with a leaky attachment and asserts render-time sanitizing, attachment redaction counts and a passing gate.

Validation

  • bun run generate:agent-plugins and bun run docs:agent-client-capabilities were regenerated. The idempotence test reproduces the committed plugins/ tree.
  • Rework, at head 2f7bd64: the focused tests pass (7 files, 97 tests): feedback-review (25 tests), integration-ownership, cli-spawn-env-isolation, agent-instruction-source, focused-skills, agent-plugin-generation and skill-references. bun run generate:agent-plugins changes only the hack-feedback SKILL.md and is idempotent. CLI typecheck reports 0 errors. CLI check reports 0 errors and 34 existing warnings, none in changed files. Biome is clean on the changed files, and privacy:check passes.
  • First pass: the focused tests passed (9 files, 106 tests): tests/feedback-review.test.ts (new), focused-skills, agent-plugin-generation, skill-references, integration-ownership, cli-spawn-env-isolation, agent-instruction-source, agent-client-capabilities and pattern-catalogue.
  • bun run --cwd packages/cli typecheck: 0 errors. bun run --cwd packages/cli check: 0 errors (existing warnings only, none in changed files). biome check on the changed tests and scripts is clean.
  • bun run privacy:check passes with the new files staged. Fixture tokens are assembled at runtime, so no credential-shaped string is committed.

Negative controls (mock sender; the sender is never called on refusal)

  • Redaction fixtures: provider tokens, bearer headers, JWTs, secret flags and assignments, URL credentials (the whole URL), PEM, age and sops ciphertext, encoded blobs, home, user, temp and tilde paths, private, routed and local URLs (Hack's public issues allowed), config values (key names kept), emails, SSH remotes, IP addresses (loopback kept) and digests. Each fixture asserts exact redaction kinds, idempotence and no remaining findings.
  • Misleading log instructions: the log claims pre-approval and names an attacker repository. The lines are removed, and a content-supplied destination or approval is refused (destination-unconfirmed, approval-not-from-user).
  • Duplicates: search terms carry no values. A same match refuses a new issue (known-duplicate) and allows a confirmed comment.
  • Reproduction status: partial reproduction is reported with its note. An unknown argv renders as unknown and is not reconstructed.
  • Denied posting: returns posting-denied, and the draft is retained unchanged.
  • Edited drafts and attachments: body or title edits, a listed attachment added after review, or a changed destination return payload-changed. A hidden attachment returns attachment-unlisted. A re-added secret or a raw-log attachment returns privacy-findings. A fresh review makes the edited payload sendable.
  • Failed-send retries: a failure or thrown error consumes the approval. Reusing it returns approval-already-used. A new approval retries, and once a send succeeds, further attempts return already-sent.
  • No built-in sender: a source scan finds no fetch, spawn, http or GitHub client in the feedback modules.
  • Rework mutation proofs: 18 further mutations were tried on the rework, and all were killed. They undid the CLI_SPEC command path, the error-code filter, the placeholder lookahead (bare and quoted), the hostname rule, the extra absolute roots, the IPv6 rule, the IPv6 clock-time guard, the Authorization rule, the 16-character digest length, sync-throw handling, the issue-number check, the URL parse and the digest's attachments. The rest covered render-time attachment sanitizing, the old broad instruction regex, the user-said-yes pattern and the agent-must-now pattern.
  • Mutation proofs (first pass): 18 mutations were tried: approval consumption, digest binding, confirmation and approval source, denial, already-sent, duplicate, privacy recheck, attachment listing, sender error handling, instruction and log removal, home paths, config values, SSH remotes, search-term values, URL allowlist, and the generator dropping the skill. Each one was caught by a failing test.

Remaining gaps

  • Client loading of the new skill in a fresh Codex, Claude or Cursor session has not been observed. The capability matrix keeps skill discovery at its existing status.
  • The sanitizer is a reference model for the documented rules and the gate tests. It is pattern-based and cannot prove a free-form report is anonymous. Public hostnames (for example api.acme.com) and bare names in prose are left to the user's review: only private, reserved and *.hack suffixes are matched, so that file names such as package.json are not redacted. The skill still requires the user to review the exact text.
  • No live issue was created. Live posting needs its own explicit authorization from the user.
  • Hosted CI has not been checked yet for head 2f7bd64.

Rework 2 (head f342e44)

The verifier reported these open defects at fdd00e40. Each one reproduced on that head with findFeedbackPrivacyIssues(...) = [] or a sent mock outcome. Each one is now fixed and has a test.

  • Placeholder bypass (blocking): the lookahead accepted ), ], }, quotes or a backtick straight after <x>, while the value kept going. A placeholder is now exempt only when it is the whole value. After <...>, only trailing punctuation may come before the value's own end character: whitespace for flags, [\s,;] for assignments, [\s,}] for JSON values, and whitespace, quotes, backtick, , or ; for Authorization values. A Bearer value that starts with < is redacted. Now redacted: API_TOKEN=<x>)…, --password <x>)…, --password <x>,…, API_TOKEN=<x>"…", <x>]…, <x>'…, password=<x>}…, token: <x>`…` and Bearer <x>)…. These still pass unchanged: (set API_TOKEN=<value>),, HACK_A=<a>,HACK_B=<b> and {"PORT": <port>,"HOST": <host>}. The verifier's exact strings, in a body the user approved for a confirmed destination, return privacy-findings.
  • Attachment-name privacy check had no test: this gap was real. A new test sends an unlisted attachment named /Users/alice/acme/dump.log and expects privacy-findings. With the name check removed, the result is attachment-unlisted. Drafts now sanitize attachment names before the body lists them, so the listing and the attachment carry the same <redacted:path>.
  • URL fragment allowlist had no test: the URL case now includes https://github.com/hack-dance/hack/issues/1#acme.secret-project.
  • Structured logs: raw-log now covers JSON records with a log key (level, ts, time, timestamp, msg, logger, log, severity) in any position. It also covers syslog (Oct 19 10:00:01 host proc[1]:), Go-style 2026/10/19 10:00:01 and clock-time prefixes. A --json envelope such as {"ok":false,"error":{...}} stays.
  • Hidden attachment listing: an attachment counts as listed only on a visible line of the ## Attachments section that starts with - `name`. That line may be followed by nothing, whitespace or :. HTML comments are removed first, including an unterminated one. A section with raw HTML outside inline code lists nothing. A name containing a backtick or a line break never counts as listed. A listing inside <!-- -->, <div hidden> or <details>, outside the section, or as a prefix only (- `dump.log`.bak) returns attachment-unlisted.
  • Lower-severity sanitizer gaps:
    • (a) db_pass, any <prefix>_KEY such as encryption_key, cookie and SESSION_ID are secret keys. bypass: and tests passed: stay.
    • (b) A ~/.hack path stays only when every part is a Hack-owned name, such as daemon/hackd.sock, projects.json or certs, or a placeholder. ~/.hack/projects/<name>/… is redacted. $HOME/…, ${HOME}/…, /opt/… (except /opt/homebrew) and /var/lib/… are redacted too.
    • (c) A host with an inner corp/internal/intranet/lan/private label is redacted with its path, for example acme-secret.internal.acme.com/admin. Names that end in a file extension, such as app.internal.ts, stay, and so does compose.local.yml.
    • (d) Secret-store envelope fields are redacted at any length: ciphertext, encrypted* and wrapped_key always, and iv, tag, nonce and salt when the value looks encoded. "tag": "latest" stays.
    • (e) These are new instruction patterns: "agents reading this", "if you are an AI", "open an issue at owner/repo", "…and paste the env", and a line that starts with paste or attach followed by secrets, tokens or credentials. "The env values were pasted into…" and "I tried to include the env overlay" stay.
  • The skill's attachment-unlisted summary now describes the visible-section rule (plugins/ regenerated).

Validation (rework 2)

  • tests/feedback-review.test.ts: 29 tests pass. Focused set: feedback-review, focused-skills, agent-plugin-generation and skill-references, 67 tests pass.
  • Mutation proofs: 37 mutations were tried, and every one was killed. They covered both verifier mutations (attachment names in the gate, SAFE_URL_FRAGMENT), the old placeholder lookahead, the per-grammar terminators (assignment, JSON), the Bearer < branch, each new secret key and the PASS word guard, both envelope rules, each new raw-log form, the ~/.hack allowlist and exemption, the /opt, /var/lib and $HOME roots and the Homebrew guard, the inner-label host rule and its file-extension guard, each of the five instruction patterns, comment stripping, the raw-HTML rule, the inline-code exception, section scoping, the prefix-only check and render-time name sanitizing.
  • gitleaks 8.30.1 (detect --log-opts=origin/next..HEAD, .gitleaks.toml): 4 commits, no leaks. Fixture values are assembled at runtime.
  • bun run --cwd packages/cli typecheck: 0 errors. bun run --cwd packages/cli check: 0 errors and the 34 existing warnings.
  • Full bun run test (local, load average 13 to 23): 6969 pass, 149 skip, 258 fail across 56 files. Every failing file was rerun serially with --timeout 60000. 42 files then passed fully, including every agent, MCP, plugin, env and registry file. The rest fail at fixed 20 or 30 second deadlines inside the tests, not at the runner timeout: native-authored-project-start (the first failing test differs each run, then cascades with "Startup fixture settlement unconfirmed"), native-compose-storage-activation-command, native-compose-retained-storage-command, native-compose-storage-fence-cost (CLI cold start returns 1 after about 35 seconds), native-compose-adoption-generation (11 tests hit 20 or 30 second fixture deadlines), native-compose-proxy-probe (2 TLS cases), native-authored-project-recovery (2) and native-authored-process-incarnation (1). None import the feedback modules. native-authored-project-start fails the same way with this change reverted to the parent commit. These are reported as load-only local failures; hosted CI is the confirming gate.

Remaining gaps (rework 2)

  • The sanitizer is still pattern-based. Public hostnames and bare names in prose are left to the user's review of the exact text.
  • A placeholder written outside a code span that is a valid HTML tag name, such as <project>, is hidden by GitHub's renderer. It is not a leak, but the rendered issue loses the placeholder. Drafts render command shapes in code spans.
  • No live issue was posted, and no client loading of the skill was observed. Linear was not updated.

Rework 2, follow-up (head b20820f)

I checked every verifier defect again on f342e444, using the verifier's exact strings. All six are fixed there:

  • The placeholder bypass cases are redacted. This includes <x>), <x>", <x>], <x>', <x>} and <x>`, plus extra cases with ., :, /, |, <y>, --token=, Bearer and x-api-key.
  • JSON, syslog and Go-style log lines are removed as raw-log.
  • An attachment listed only inside an HTML comment is refused.
  • Gaps (a) to (e) are redacted.
  • I re-applied the verifier's two mutations: removing attachment names from the gate, and removing SAFE_URL_FRAGMENT. A test now catches each one.

This follow-up adds two commits:

  • 5b9613e2 test fixtures: 5b9613e2 was committed earlier but not pushed. It fixes the hosted Test failure on f342e444. On that head, bun run privacy:check flagged the fixture /Users/alice/acme/dump.log. The fixtures now use /Users/example/... and neutral host and user names.
  • b20820fc hidden attachment listing (new defect):
    • Problem: the gate checked for raw HTML only inside the ## Attachments section. A <details> opened before the heading folded the whole section, and its listing still counted, so the gate allowed the send. <div hidden> around the heading did the same. \`<details>` (an escaped backtick) and <details title="">` hid the tag from the inline-code filter.
    • Fix: any element that can hide Markdown, anywhere in the body and outside inline code, now cancels the listing. Escaped backticks no longer open code spans. The section's tag check now uses CommonMark tag names, so a <redacted:path> placeholder in the section is treated as text, not HTML.
    • Skill text: the skill's attachment-unlisted summary describes the new rule. plugins/ is regenerated.

Validation (rework 2 follow-up)

  • tests/feedback-review.test.ts: 29 pass. The focused set (feedback-review, focused-skills, agent-plugin-generation, skill-references) has 67 passing tests.
  • Mutation proofs: 4 new mutations, all killed:
    • removing the body-wide hiding-element check (the "details around the heading" case fails);
    • removing escaped-backtick handling (the "escaped backtick before a tag" case fails);
    • restoring the old generic HTML_TAG (the placeholder listing becomes unsendable);
    • removing the in-section tag check (the "custom element in the section" case fails).
  • bun run privacy:check: ok.
  • gitleaks 8.30.1 (gitleaks git --config .gitleaks.toml --redact --log-opts=origin/next..HEAD): 6 commits scanned, no leaks.
  • bun run --cwd packages/cli typecheck: 0 errors. bun run --cwd packages/cli check: 0 errors, plus the 34 existing warnings.
  • Full bun run test: run locally with load average 13 to 67. The result was 6988 pass, 149 skip and 239 fail. I reran each failing file serially with --timeout 60000, and 40 of 47 files then passed fully. Those include every agent, MCP, plugin, env, registry and global-command file. The agent-plugin-generation failures under load were spawn exit 127.
    • Still failing serially, all native runtime suites hitting fixed 20 to 60 second fixture deadlines: native-compose-adoption-generation (110), native-authored-project-start (11), native-compose-down-command (8), native-authored-project-recovery (2), native-compose-file-command (2), native-compose-vm-file-finalization (2) and native-authored-process-incarnation (1).
    • None of these import the feedback modules. Only skill-references, feedback-skill, the generator and three agent test files do.
    • I am reporting these as load-only local failures. Hosted CI is the confirming gate.

Remaining gaps (unchanged)

  • The sanitizer is pattern-based. Public hostnames and bare names in prose are left to the user's review.
  • No live issue was posted, and no client loading of the skill was observed. Linear was not updated.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

roodboi and others added 6 commits October 19, 2026 11:46
Add a plugin-only hack-feedback skill that drafts reports about Hack
itself offline: observed and expected behavior, one honest reproduction
status, evidence tagged observed/source/reported/inferred, a friction
category, sanitized duplicate search and an attachment list. Posting
needs a destination the user confirms and an explicit yes for the exact
payload, and goes through the user's own tooling; Hack ships no sender.

The skill, its issue-template and examples references, and the new
.github/ISSUE_TEMPLATE/hack-feedback.md render from one typed contract
in src/agents/feedback-review.ts through the plugin generator. The same
module holds the reference sanitizer and the posting gate the skill
describes, exercised with a mock sender over synthetic secret, path,
URL, config, log-instruction, duplicate, partial-reproduction, denial,
edit and failed-send fixtures (HACK-1138).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Duplicate-search terms keep only the leading tokens that resolve to a
  CLI_SPEC command path (injected) and registered HACK_ERROR_CODES, so
  positional project, service and script names never become terms.
- Placeholder exemptions apply only to a complete `<...>` value ending at
  whitespace, punctuation or end of text; `KEY=<x>leak` is redacted.
- New `hostname` redaction kind for routed `*.hack` and private-suffix
  hosts without a scheme; paths under /root, /srv, /Volumes, /workspace and
  other roots plus Windows drive paths; IPv6; truncated (16+ hex) digests;
  `Authorization: token ...` headers.
- Public-URL allowlist parses the URL and rejects dot segments, encoded
  dots, queries and free-text fragments.
- Embedded-instruction filter targets addressed instructions and approval
  claims instead of any "send the report" phrase.
- attemptFeedbackSend treats a synchronous sender throw as a failed
  attempt and consumes the approval; comment destinations need a positive
  integer issue number.
- Tests cover each case, including attachment-only edits after review and
  attachment sanitizing at render time; 18 targeted mutations all killed.

Refs HACK-1138

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The feedback-review fixture embedded a literal OpenSSH private-key armor
line, which gitleaks flags as a private key. Build the armor at runtime
like the file's other token-shaped fixtures, and record the fingerprint
of the earlier branch commit that still contains the literal.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A placeholder is exempt only when it is the whole value: after `<...>`
only trailing punctuation may come before the value's own terminator, so
`API_TOKEN=<x>)leak`, `--password <x>)leak` and `token: <x>`leak`` are
redacted. Bearer values that start with `<` are redacted too.

The sanitizer now removes JSON, syslog, Go-style and clock-prefixed log
lines; redacts `db_pass`, `*_key`, cookie and session-id values, secret-
store envelope fields at any length, project paths under `~/.hack`,
`$HOME`, `/opt` and `/var/lib`, and hosts with a private inner label; and
drops more agent-addressed instructions.

The send gate counts an attachment as listed only on a visible line of
the Attachments section, so a listing inside an HTML comment or raw HTML
is refused. Drafts sanitize attachment names before listing them. New
tests cover the attachment-name privacy check and the URL fragment
allowlist, which no test exercised before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The privacy check rejects a macOS home path with a personal-looking user name. Use the file's existing /Users/example fixture style and neutral host and user names in the new log and attachment cases.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The send gate only looked for raw HTML inside the Attachments section, so a
`<details>` opened before the heading folded the whole section and its
listing still counted. Any element that can hide Markdown, anywhere in the
body and outside inline code, now cancels the listing. Escaped backticks no
longer hide a tag, and the section's tag check uses CommonMark tag names, so
a `<redacted:path>` placeholder in the section is text, not HTML.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
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.

1 participant