mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-07 06:16:06 -10:00
docs: define module ownership and shared change review (#17087)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Coding agents extend connectors and shared control-plane services. > - Provider details can leak into shared services during these changes. > - Moving code into a new file does not explain why shared behavior must change. > - This pull request adds module ownership and shared-change review rules. > - The rules help agents justify the change before implementation and review. ## Linked Issues or Issue Description **Issue type** Missing documentation. **Where is the issue?** Repository architecture rules and the connector authoring playbook. **What's wrong?** Subsystem rules describe some ownership boundaries. The repository lacks a general rule for deciding which behavior belongs in a feature module, what shared contracts must change, and how to review the resulting scope. An integration can therefore add provider-specific logic to shared code or bundle large platform changes into one connector PR. **Suggested fix** Add a repository-wide rule and load it in Greptile. Apply the rule during connector planning and PR preparation. Keep provider protocol behavior with its owner. Require a reason for shared changes and checks through affected consumers. Related public context: [Notebook connector PR #16984](https://github.com/paperclipai/paperclip/pull/16984). This PR changes instructions only. ## What Changed - Add a concise module-boundary rule for ownership, narrow contracts, shared controls, delivery scope, and verification. - Register the rule as repository-wide Greptile context. - Add planning and PR review steps to the connector playbook. Keep tests and docs with their behavior. ## Verification - Passed rule frontmatter, Greptile registration, referenced-file, and playbook-link checks. - Passed `git diff --check origin/master...HEAD`. - Reviewed the rule against the relevant connector failure and allowed cases: small dispatch branches, one-consumer shared capabilities, and unrelated fixes. - Fresh Greptile and Paperclip reviews both scored commit `759572fe503e1b6dfa9527f53de6c326a2c3bff6` 5/5. No actionable findings or unresolved review threads remain. - Final current-commit GitHub checks: 55 successful, 1 neutral, and 2 skipped; no pending or failed checks. Both required `ci / verify` and `ci / e2e` gates passed. - Initial CI attempt: the build and runtime boot smoke test passed. The spot runner received a shutdown signal during the clean public npm install check, canceling that step and failing the build job. [Interrupted job log](https://github.com/paperclipai/paperclip/actions/runs/39601811838/job/119228042468). Retried failed jobs once. Attempt 2 passed the complete build, runtime boot, and clean-install verification. The initial runner interruption remains recorded here. - Full local application checks are unavailable: this checkout has no workspace dependencies, and `pnpm -r typecheck` stopped during pnpm bootstrap because registry fetches failed and the requested pnpm version could not be verified. Local application tests and build were not run. GitHub checks must pass before merge. ## Risks - The rule could cause unnecessary abstractions or extra PRs if applied mechanically. It permits small dispatch branches and useful capabilities with one consumer. It rejects arbitrary file counts and unrelated restructuring. - Prose and metadata checks do not prove that future agents will follow the rule. Hosted review receives it when this branch is reviewed. ## Model Used - OpenAI Codex: `gpt-6-astra` authored the guidance; `gpt-6.1-sol` prepared delivery. Both used reasoning, filesystem tools, and GitHub tooling. Context window size was not recorded in the available model settings. ## 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 (focused documentation and metadata validation; application checks unavailable as stated above) - [x] I have added or updated tests where applicable (no runtime change; targeted structural checks cover the changed instructions) - [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 <[email protected]>
This commit is contained in:
1 parent
c6ee96a3df
commit
14afc12124
3 files changed
+66
-3
No files matched your search
@@ -16,6 +16,10 @@
|
||||
"path": "rules/product-harness-responsibility.mdc",
|
||||
"description": "Product requirements and review criteria for harness responsibility, minimal user-facing internals, truthful progress, and human decisions."
|
||||
},
|
||||
{
|
||||
"path": "rules/module-boundaries.mdc",
|
||||
"description": "Module ownership, justification of shared contract changes, delivery scope, and verification across affected consumers."
|
||||
},
|
||||
{
|
||||
"path": "rules/runtime-dependency-boundaries.mdc",
|
||||
"description": "Dependency cost, responsiveness, and verification requirements for changed server, adapter, and runner execution paths.",
|
||||
|
||||
@@ -544,7 +544,16 @@ This is the implementation sequence an autonomous coding agent should follow.
|
||||
task. Do not reset, rewrite, or format unrelated files.
|
||||
4. Decide whether the request is research-only, definition-only, or a new
|
||||
runtime capability. A store card is not evidence of runtime support.
|
||||
5. Write acceptance criteria before editing. At minimum: actionable store card,
|
||||
5. Apply [Module boundaries](../../rules/module-boundaries.mdc) before extending
|
||||
shared code. Identify what the existing connection system already supports
|
||||
and which requested features need a new platform capability. Keep new
|
||||
connector-specific protocol behavior in
|
||||
`server/src/services/connectors/<provider>/`; catalog definitions, branding,
|
||||
and registration belong in their established locations. Shared gateway,
|
||||
OAuth, and runtime code should use domain contracts, without interpreting
|
||||
provider tool names or response formats. Separate ordinary MCP calls from
|
||||
optional helpers such as local-file publishing when assessing the footprint.
|
||||
6. Write acceptance criteria before editing. At minimum: actionable store card,
|
||||
setup completes, credentials are vaulted, tools list, one safe read runs,
|
||||
refresh/reconnect works, revoke blocks use, and no secret appears in API or
|
||||
logs.
|
||||
@@ -1293,8 +1302,12 @@ unrelated worktree change, or temporary evidence file is staged.
|
||||
|
||||
### Phase 12: Prepare and submit the pull request
|
||||
|
||||
1. Keep commits scoped. A typical split is definition/branding, generic runtime
|
||||
capability, and tests/docs. Do not split generated output from its source.
|
||||
1. Recheck [Module boundaries](../../rules/module-boundaries.mdc). Explain each
|
||||
substantive shared change in the PR: the missing capability, why the existing
|
||||
contract is insufficient, affected consumers, and behavior with the connector
|
||||
absent or disabled. Separate substantial platform capabilities into reviewable
|
||||
slices with explicit dependencies when appropriate. Keep tests and docs with
|
||||
their behavior and generated output with its source.
|
||||
2. Do not commit `pnpm-lock.yaml`; GitHub Actions owns it in this repository.
|
||||
3. Read `.github/PULL_REQUEST_TEMPLATE.md` immediately before writing the PR
|
||||
body.
|
||||
|
||||
@@ -0,0 +1,46 @@
|
||||
---
|
||||
description: Keep behavior with its owning module and justify changes to shared contracts and delivery scope.
|
||||
alwaysApply: true
|
||||
---
|
||||
|
||||
# Module boundaries
|
||||
|
||||
Apply to new or materially changed behavior across modules. Follow subsystem
|
||||
ownership rules; unrelated code need not be reorganized.
|
||||
|
||||
- **Choose ownership first.** Identify the owner, existing contract, and missing
|
||||
shared capability in the task's existing plan or PR. No separate document or
|
||||
approval is required. Convenient local variables do not justify orchestrator logic.
|
||||
- **Isolate implementation details.** Provider protocols, payload parsing,
|
||||
workarounds, and feature sequencing belong with their owner. Shared code consumes
|
||||
normalized domain contracts, not provider payloads or SDK types. Small registries
|
||||
and dispatch branches may select implementations and delegate behavior.
|
||||
- **Keep contracts narrow.** Reuse extension points; add only what the workflow
|
||||
needs. Pass required dependencies, not the whole service context. Moving code
|
||||
or renaming interfaces does not remove coupling. A coherent shared contract
|
||||
should survive provider removal or substitution. One consumer can justify it;
|
||||
speculative reuse or another fixture alone cannot. Preserve provider differences
|
||||
rather than forcing a universal interface.
|
||||
- **Preserve invariant ownership.** Authorization, company isolation, audit,
|
||||
transactions, and resource ownership retain their existing owners. Adapters
|
||||
must neither bypass controls nor create parallel enforcement paths.
|
||||
- **Explain shared changes.** For each substantive change outside the owner,
|
||||
explain the capability, why existing contracts are insufficient, affected
|
||||
consumers, and behavior with the feature absent or disabled. Group related files.
|
||||
Catalog entries, branding, schemas, and registration may stay in established
|
||||
locations without teaching shared execution the provider protocol.
|
||||
- **Separate ownership from delivery scope.** Even well-factored platform work
|
||||
may need a separate reviewable slice or explicit PR-stack dependency to clarify
|
||||
effects on existing consumers. Keep tests and docs with behavior. Preserve
|
||||
requested features and delivery constraints; avoid unusable intermediate states
|
||||
and arbitrary PR/file counts.
|
||||
|
||||
**Verify:** Exercise observable behavior through new and relevant existing
|
||||
consumers, including affected absent/disabled states—not just helper calls.
|
||||
A different-protocol fixture can expose coupling but proves neither production
|
||||
compatibility nor the abstraction's necessity.
|
||||
|
||||
**Review:** Flag misplaced behavior, leaked private representations, duplicated
|
||||
invariant ownership, and unexplained shared changes; name the proper owner and
|
||||
contract. Recheck necessity and scope after refactoring. Identifier searches,
|
||||
line counts, and passing CI alone do not establish sound boundaries.
|
||||
Reference in new issue
Block a user