Skip to content

chore: rework contribution policy to issue-first, with CI enforcement - #188

Merged
christosgkoros merged 1 commit into
mainfrom
chore/contribution-policy-PSTAPI-1647
Aug 19, 2026
Merged

chore: rework contribution policy to issue-first, with CI enforcement#188
christosgkoros merged 1 commit into
mainfrom
chore/contribution-policy-PSTAPI-1647

Conversation

@christosgkoros

@christosgkoros christosgkoros commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Refs PSTAPI-1647

Why

CONTRIBUTING.md invites pull requests we can never merge. Its "Safe to edit (PRs welcome directly)" list names paths the sync overwrites — every entry on it except the generated tool files. PR #138's hand-written unit tests were rewritten by abebf8b three commits after merge.

Two mechanical traps compound it for a first-time contributor:

  • Fork CI could not go green. test.yml passed secrets.POSTMAN_API_KEY, which is empty for fork PRs, and direct.test.ts spawned the real server with no guard while src/env.ts calls process.exit(1) without a key. A valid docs PR got an unexplained red X.
  • The documented commands didn't work. pnpm test is bare vitest (watch mode), so pnpm build && pnpm test && pnpm lint hangs. And dist/ is committed, so "run pnpm build before pushing" produces a ~180-file diff that contradicts the guide's own "one focused change per PR" rule.

This reframes the guide around issues as the primary contribution path, keeps docs and repository tooling genuinely mergeable, and enforces the boundary in CI. It says that tool definitions and the server implementation are synced from Postman's internal source of truth, and nothing about how that pipeline works.

What's in here

Docs

  • Rewrite CONTRIBUTING.md: state the constraint once, early, then spend the document on what contributors can do. Replace the incorrect "safe to edit" list with an open-vs-synced table. Correct the test commands, tell people not to run pnpm build, add the missing --learn flag.
  • Extract SECURITY.md and CODE_OF_CONDUCT.md to root files so GitHub surfaces them (Security tab, issue sidebar, community profile); add SUPPORT.md.
  • Add a Contributing section to README.md, which never linked CONTRIBUTING.md.

Enforcement

  • .github/synced-paths.json as the single source of truth, so docs and workflow can't drift.
  • synced-paths-guard.yml comments once on fork PRs touching synced paths, applies a synced-path label, and retracts both if the PR is split. pull_request_target with no checkout of PR-head code, config read from the base commit, actions/github-script only — no third-party actions in a job holding a write token. Advisory, not blocking.
  • Issue forms for bugs, tool requests, and docs, each with a Proposed solution field so design proposals keep flowing in. Blank issues disabled.
  • A PR template that asks about synced paths up front.

CI and tooling

  • Split test.yml into a fork-safe unit job (lint, typecheck, unit tests, no secrets) and an integration job gated on same-repo PRs and pushes. Adds permissions: contents: read and --frozen-lockfile.
  • Add test:ci, test:unit, typecheck scripts.
  • Untrack .idea/workspace.xml and extend .gitignore.

Verification

The new-contributor path passes with POSTMAN_API_KEY unset:

pnpm lint PASS
pnpm typecheck PASS
pnpm test:unit PASS — 33 tests
pnpm test:ci PASS — 2 passed, 2 skipped, 0 failed

All 4 issue forms and both workflows parse; the guard's embedded JS passes node --check. Matcher tested against real data: sync commit b361515 → 261/261 hits, docs-only PR → 0, near-misses (srcfoo.ts, docs/src/x.ts, vitest.config.ts.bak) → 0.

Reviewer notes

  • describe.skipIf alone was not enough for direct.test.ts. The suite imports PostmanAPIClient, which imports src/env.ts, which calls process.exit(1) at module load — before skipIf can skip anything. Making that one import lazy is what actually fixes it. This file is synced, so the same change needs to land upstream to survive. test.yml is what keeps fork CI green regardless.
  • This is deliberately one PR rather than the five the plan sequenced, and it does cut against the one-change-per-PR rule it adds. The parts are interdependent — the package.json scripts have to exist before the docs and CI reference them.
  • src/tests/integration/learnToolset.test.ts is intentionally not included — pre-existing untracked file, not part of this change. Note it has a type error that tsconfig.json's exclude: ["src/tests/**"] currently hides from pnpm typecheck.

Before merging

  • SECURITY.md has no acknowledgement SLA — currently "we'll acknowledge and keep you updated", pending real wording from Postman security.
  • Create labels synced-path and needs-triage.
  • Enable private vulnerability reporting — the advisories link in ISSUE_TEMPLATE/config.yml 404s until then.
  • Confirm the org Actions policy permits pull_request_target and actions/github-script.
  • Branch protection: if it requires a check named test, the rename to unit / integration breaks it.
  • --frozen-lockfile is new — it will fail if a sync bumps package.json deps without regenerating pnpm-lock.yaml. That's the point, but the first red build shouldn't surprise anyone.

Follow-ups (not in this repo)

  • Land the direct.test.ts guard and lazy import upstream, or the next sync reverts them.
  • src/env.ts:13-15 references shared/env.ts, a path that doesn't exist here — an internal-monorepo reference in public source. Only fixable upstream.
  • Consider a @generated banner in the upstream tool templates; no generated file currently carries any in-file warning.
  • Agree a written maintainer rule for "convert PR → credited issue", so the credit promise in the new docs is actually kept.

Generated with Claude Code

CONTRIBUTING.md invited pull requests we can never merge. Its "Safe to edit
(PRs welcome directly)" list named paths the sync overwrites -- every entry on
it except the generated tool files. PR #138's hand-written unit tests were
rewritten three commits after merge.

Two mechanical traps compounded it: fork CI could not go green (test.yml passed
a secret that is empty for forks, and direct.test.ts spawned the real server
with no guard), and the documented `pnpm build && pnpm test && pnpm lint` chain
hung because `pnpm test` is bare vitest in watch mode.

Reframe the guide around issues as the primary contribution path, keep docs and
repository tooling genuinely mergeable, and enforce the boundary in CI.

Docs
- Rewrite CONTRIBUTING.md: state once that tool definitions and the server
  implementation are synced from Postman's internal source of truth, then spend
  the document on what contributors can do. Replace the wrong "safe to edit"
  list with an open-vs-synced table. Correct the test commands, tell people not
  to run `pnpm build` (it recompiles the committed dist/), and add the missing
  --learn flag.
- Extract SECURITY.md and CODE_OF_CONDUCT.md to root files so GitHub surfaces
  them; add SUPPORT.md to route usage questions away from the bug tracker.
- Add a Contributing section to README.md, which never linked CONTRIBUTING.md.

Enforcement
- .github/synced-paths.json as the single source of truth, so the docs and the
  workflow cannot drift.
- synced-paths-guard.yml comments once on fork PRs touching synced paths,
  applies a `synced-path` label, and retracts both if the PR is split. Uses
  pull_request_target with no checkout of PR-head code and reads its config
  from the base commit; actions/github-script only, no third-party actions.
  Advisory, not blocking.
- Issue forms for bug reports, tool requests, and docs, each with a Proposed
  solution field so design proposals keep flowing in. Blank issues disabled.
- A PR template that asks about synced paths up front.

CI and tooling
- Split test.yml into a fork-safe `unit` job (lint, typecheck, unit tests, no
  secrets) and an `integration` job gated on same-repo PRs and pushes. Add
  permissions: contents: read and --frozen-lockfile.
- Add test:ci, test:unit, and typecheck scripts.
- Guard direct.test.ts on POSTMAN_API_KEY. describe.skipIf alone was not
  enough: the suite imports PostmanAPIClient, which imports src/env.ts, which
  calls process.exit(1) at module load -- before skipIf can skip anything. Make
  that one import lazy so the guard works. `vitest run` now passes with no key
  (2 passed, 2 skipped). This file is synced, so the same change needs to land
  upstream to survive; test.yml is what keeps fork CI green regardless.
- Untrack .idea/workspace.xml, which exposed a maintainer's local path, and
  extend .gitignore to cover editor and local agent config.

Refs PSTAPI-1647

Co-Authored-By: Claude <noreply@anthropic.com>
@christosgkoros
christosgkoros merged commit 7644639 into main Aug 19, 2026
10 checks passed
@christosgkoros
christosgkoros deleted the chore/contribution-policy-PSTAPI-1647 branch August 19, 2026 07:31
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