Commit Graph
3 Commits
Author SHA1 Message Date
DottaandPaperclip 2f0c485dec fix(skills): ship the completion helper with the installed skill (#15554)
## Thinking Path

> - Paperclip manages work for AI agents.
> - Legacy agents use the Paperclip skill to save task status and
comments.
> - The skill names a script relative to the task workspace.
> - That script exists only in the Paperclip source repository.
> - Agents in other workspaces can hit a missing command or search for
it.
> - This PR ships the helper inside the skill and uses the installed
skill path.
> - The repository command remains available through a forwarding
wrapper.

## Linked Issues or Issue Description

Fixes #9527. Refs #15548 for the preceding runtime checkout guidance.
Related: #6052 addresses LF line endings for the repository helper; this
change addresses helper delivery and path resolution.

## What Changed

- Bundle the existing issue update helper with the Paperclip skill.
Preserve its HTTP checks, echoed-status check and two-attempt limit.
- Resolve the command from the installed skill directory. Use a verified
PATCH when that path is unavailable, without searching the filesystem.
- Keep the repository command as a wrapper that works from any
directory.
- Test shell execution and exact status/comment payloads through both
provider skill-home layouts, including paths with spaces.
- Add helper sources and existing verification tests to stock-harness
admission. Record an absent historical helper explicitly. Add the
missing declaration for the admission fingerprint export.

## Verification

- Complete directly affected source suites: 30 tests pass. They cover
skill delivery, preserved multiline comments and links, authentication
headers, empty responses, mismatched status, transient retries and
definitive rejections.
- Product E2E typecheck passes. Support suites: 1,835 Vitest tests pass,
one is skipped; 128 Node tests pass.
- Full local build and workspace typecheck pass.
- Full local repository tests are not claimed as passed. Embedded
PostgreSQL was unavailable in this worktree during the preceding task;
Linux CI will run the repository gates.
- The authorized matched Codex/Claude comparison is pending. It uses the
existing assigned-skill case and original oracle, one initial attempt
per profile and variant.
- CI and a fresh Greptile review are pending. Keep this PR in draft
until readiness gates complete.

## Risks

- Correct path resolution depends on the harness supplying the installed
skill path. The instructions use verified PATCH when that path is
unavailable.
- The helper still requires Bash, curl and jq. Its existing retry and
response-verification behavior is unchanged.
- Tests use the shared skill-directory symlink mechanism and an HTTP
fixture. Real provider completion behavior still requires the bounded
live comparison.
- This fix does not redesign native completion, legacy recovery or
ambiguous transport handling.

## Model Used

OpenAI Codex, GPT-6 family. The exact model build and context window are
not exposed in this session. Used code editing, shell tools and test
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
- [ ] All Paperclip CI gates are green
- [ ] 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>
2026-10-08 07:21:30 -05:00
Devin Foley 633e102971 fix: verify issue-update writes instead of inferring success (#12051)
## Thinking Path

> - Paperclip is the open source app people use to manage AI agents for
work
> - Agents report task state to the control plane with `PATCH
/api/issues/{id}` at the end of each heartbeat
> - On remote sandbox targets those writes cross a relay that can fail
at the connection level
> - An agent that pipes its status curl through `head` cannot see that
failure; the write is lost but the run reports success
> - The issue then stays `in_progress` with no disposition, and the
missing-disposition recovery must repair it
> - This pull request makes the issue-update helper verify every write,
and it teaches the shared skill to require verified writes
> - The benefit is that a lost status write becomes a visible, retried
failure instead of a silent success

## Linked Issues or Issue Description

No public issue exists for this defect. The description below follows
the bug report template.

**What happened?**

A sandboxed heartbeat run answered its issue in a comment. It then sent
`PATCH /api/issues/{id}` with `status: done` through `curl -sf ... |
head -c 400`. The relay dropped the connection. The `-f` flag suppressed
the error output, and the pipe replaced curl's exit code with the exit
code of `head`. The agent saw empty output and exit 0. It reported the
write as an "empty 2xx" success and exited. The issue stayed
`in_progress`, and the successful-run recovery had to close it in a
corrective run.

**Expected behavior**

A status write that does not reach the server must surface as a failure.
The helper script must retry transient failures. It must exit non-zero
when the write is unconfirmed. Skill guidance must forbid write patterns
that hide failures.

**Steps to reproduce**

1. Point `PAPERCLIP_API_URL` at an endpoint that drops connections
intermittently.
2. Finalize an issue with `curl -sf -X PATCH
"$PAPERCLIP_API_URL/api/issues/$ID" -d '{"status":"done"}' | head -c
400`.
3. Observe exit code 0 with empty output while the server never received
the PATCH.

## What Changed

- `scripts/paperclip-issue-update.sh` now captures `%{http_code}`,
retries a retryable failure (connection-level, 429, 5xx) once — two
attempts total, which matches the shared bounded-write-retry rule —
rejects an empty 2xx body, and confirms the response echoes the
requested status before it exits 0.
- Failure output states plainly that the write was NOT saved, so the
calling agent reports it accurately.
- `skills/paperclip/SKILL.md` Step 8 adds a required "Verify writes —
never infer them" rule: a successful PATCH always returns the updated
issue JSON, disposition writes must never run through `head`/`tail`
pipelines, and an unconfirmed write must be reported as FAILED.
- `server/src/__tests__/paperclip-skill-utils.test.ts` pins the new
skill rule; a new `paperclip-issue-update-helper.test.ts` exercises the
helper's behavior end-to-end.

## Verification

- `bash -n scripts/paperclip-issue-update.sh`
- `server/src/__tests__/paperclip-issue-update-helper.test.ts` runs the
helper end-to-end against a local HTTP server: confirmed-echo success
(exit 0), empty 2xx (exit 1), wrong echoed status (exit 1), 422 reject
(exit 1, exactly one request), 503 then success (two requests),
connection refused (two attempts, then exit 1 with a "NOT saved"
report).
- `npx vitest run
server/src/__tests__/paperclip-issue-update-helper.test.ts
server/src/__tests__/paperclip-skill-utils.test.ts
server/src/__tests__/cli-invocation-safety.test.ts` — 50 passed.

## Risks

- Low risk. The success-path output is unchanged (the updated issue
JSON).
- The helper now exits non-zero on unconfirmed writes. Callers that
previously missed silent failures now see explicit errors. That is the
intended behavior change.
- The single retry re-sends the PATCH after a retryable failure. If the
first request committed and only its response was lost, an attached
comment can post twice. The duplicate is visible and benign; the prior
behavior lost the write silently.

## Model Used

- Claude Fable 5 (Anthropic), model id `claude-fable-5`, extended
thinking enabled, agentic tool use via Claude Code (CLI harness), 200k
context window.

## 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
2026-08-23 16:56:51 -07:00
Dotta 8bdf4081ee chore: improve worktree tooling and security docs 2026-04-10 22:26:30 -05:00