docs(skills): encode integration-hardening learnings into the authoring skills - #7271
docs(skills): encode integration-hardening learnings into the authoring skills#7271waleedlatif1 wants to merge 7 commits into
Conversation
…ng skills
Eight traps found across a 15-PR integration-hardening sweep, each written
into the skill that owns it and cross-referenced rather than duplicated.
add-tools
- Reserved param names. The shared transport reads `timeout`, `proxyUrl`, and
`method` off `params` before `request` sees them; `timeout` is its own HTTP
deadline in milliseconds, so Daytona's documented 10-second sandbox timeout
aborts the call after 10ms.
- Path traversal. `encodeURIComponent` does not stop `.`/`..` — they are
unreserved and the URL parser removes dot segments after decoding. Documents
when each of the three `tools/url-path.ts` helpers applies, and why
`params.x?.trim()` guards `undefined` rather than the type.
add-block
- Omitting a key from `tools.config.params` does not drop it; the executor
merges the patch over the raw inputs, so clearing a key needs an explicit
`undefined`.
- Renaming a subBlock id orphans saved workflow state. Rename the tool param
and map it; `_removed_` migrations cover genuine removals.
- Declared `outputs` do not drive variable resolution — the resolver walks the
runtime object, so changing an output's shape breaks references that were
never declared.
validate-integration
- Path-safety harness design: enumerate (tool, param) pairs, fuzz one at a time,
assert named rejection rather than path shape, probe conditional and presence
branches, and assert the skip ledger is empty.
- Test files are type-checked by nothing — tsconfig excludes them and Vitest
transpiles without checking.
- Replaces the two checklist lines that taught the now-known-defective
`${params.id.trim()}` path pattern.
add-integration gets pointers only.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThe PR adds integration-authoring guidance based on recurring hardening failures.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| .agents/skills/add-block/SKILL.md | Documents durable subBlock IDs, explicit parameter clearing, and backward-compatible output transformations. |
| .agents/skills/add-integration/SKILL.md | Adds concise cross-references for reserved parameters, path safety, and block-state compatibility. |
| .agents/skills/add-tools/SKILL.md | Adds detailed transport-reservation and URL-hardening guidance with an explicit precondition for helpers not yet available on staging. |
| .agents/skills/validate-integration/SKILL.md | Replaces unsafe path guidance and adds comprehensive validation patterns for traversal guards and trustworthy tests. |
Reviews (5): Last reviewed commit: "docs(skills): correct the Enrow billing ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
… a succeeding one The most valuable learning of the sweep, and the one every other check missed — sixteen PRs' own suites, both review bots, and the author. Trimming a path identifier looks strictly safer. It is not when the identifier previously went out through a bare encodeURIComponent and reaches a destructive endpoint: box_sign_cancel_request went from a 404 no-op to cancelling a real signature request, and delete_r2_bucket from naming no bucket to destroying prod-data. BigQuery's delete_dataset and delete_table had the same shape on projectId. Records the reasoning error that hid it: "trimming is the helper's contract at all 137 sites" is an average, and the question is an intersection — parameters whose normalisation actually changed, crossed with irreversible operations. On that PR the answer was one of 137. The resolution is strictUrlPathSegment, argued from the values (no legitimate id carries surrounding whitespace, and the previous behaviour was already a clean failure), not from consistency. Two smaller ones folded in: - safeUrlPath rejects only a truly empty path component, never a whitespace-only one. Git tracks a file and a directory named only spaces, and the parser never removes %20%20%20. - A test that calls a function directly can pass while the wrapper does the opposite. executeTool catches a postProcess throw and restores the submit response, so eleven green Enrow failure-path tests sat over a production success: true.
|
Added the late learning — the most valuable one this effort produced — plus the two smaller ones. Pushed as
|
The most-repeated defect of the whole effort — four separate instances on
one PR, each a blanket tolerance that made an assertion unable to fail.
Promoted from a note to a first-class rule in the harness-design step.
A tolerated throw must be tolerated BY NAME, in an explicit allowlist,
with the reason recorded. A blanket `catch { return }` converts every
case it covers from tolerated to untested. The line that keeps the rule
usable: tolerating a failed probe during discovery is legitimate, since
probing a guarded param is meant to throw — tolerating a throw inside an
assertion is the bug.
The fourth instance earns its own paragraph because it fails in the
opposite direction from everything else this sweep was about: swallowing
the throw meant the origin, prefix and inert-probe assertions never ran,
so a guard that OVER-tightened passed silently. A path-safety suite that
only catches under-guarding is half a suite. The resolution shape —
enumerate every pair against every inert value, measure which legitimately
throw, then make a throw a failure unless the param is in an explicit
strictlyValidated list — is written out, with the measured answer of ten
pairs (Supabase table and functionName).
Adds "every new assertion is verified red before it is kept" to the
checklist, which is the practice that would have caught all four.
Also groups the type-check and bypassed-wrapper notes under one
"Your tests can lie to you" heading, and renumbers the path-safety step
to 9 (it collided with Memory Load Safety).
|
Promoted the assertion-that-cannot-fail pattern to a first-class rule. Pushed as
|
|
@greptile review |
|
@cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
…counterweight
Nine recorded instances now, not four, and the three newest are each a
different shape that does not look like a catch:
- A namesParam matcher that was a substring scan, so "projectId cannot
have leading whitespace" satisfied the assertion for param `id` and
"pathological failure" satisfied `path` — a guard naming the WRONG
identifier passed the very check meant to catch that.
- An assertion that guarded itself out of existence:
`if (serialized?.includes('projectId')) { expect(...) }`.
- describe.each over a derived array a rename had silently emptied, so a
whole block vanished emitting neither tests nor failures.
Adds the counterweight rule: assert exact error text and exact encoded
output, never a bare toThrow(). Three upstream changes to url-path.ts
landed underneath a downstream suite and only the exact assertions
noticed — trimming dropped, !segment.trim() narrowed to !segment, and a
rebase rewording "cannot have" to "must not have".
Review fixes:
- Qualifies the subBlock-rename mapper example. check-block-registry
narrows to required + user-only params and demands a subBlock key equal
to the tool param id, which a rename-at-execution mapper does not
satisfy. (cubic, correct.)
- Settles branch/ref explicitly: GitHub's branches route is greedy on its
final parameter, so `feature/api` takes safeUrlPath and a %2F would
404. safeEncodedUrlPathSegment is for a non-greedy single value such as
a label name. (cubic claimed the opposite; the shipped tools disagree.)
- Adds an availability note and converts every citation into an unlanded
path-safety file from file:line to module + symbol. Those branches are
actively rebasing — strict-url-path.ts has already been deleted and its
symbols folded into url-path.ts — so a line number is stale on arrival.
Exact file:line is kept for everything that is on staging.
There was a problem hiding this comment.
Review completed against the latest diff
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…qualifies Greptile re-raised at P1 that an author following this section cannot import the helpers it prescribes. The note existed but sat six paragraphs below the table, so it read as a footnote rather than a precondition. It now leads the section, and says what to DO rather than only what is missing: add the helper to url-path.ts with the semantics specified here, never hand-roll a local encoder at the call site.
|
@greptile review |
|
@cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…g assertions
Instances 10 and 11, both with a different CAUSE from the first nine —
which is why auditing catch blocks and filters did not find them:
- Fixture drift. `expect(serialized).not.toContain(' my-project ')` was
written when the fixture was padded; a strict guard made padding throw,
the fixture was unpadded, and the assertion stayed, asserting the
absence of a string that can no longer occur. Teaches the better fix —
derive one literal from the other so a test named "agrees" asserts
agreement rather than two constants that happen to match.
- A globally-mocked dependency. vitest.setup.ts:112 stubs
@/tools/registry as `{ tools: {} }`, so a guard iterating the registry
passes over an empty set; four real failures only reproduced after
vi.unmock.
Review fixes, all four valid and all four my own guidance failing its own
standard:
- Step 9 recommended `toThrow(new RegExp(paramName))` three paragraphs
above the section documenting the substring-matcher trap. Replaced with
the capture-and-assert form the reference harness actually uses.
- The exactness rule recommended `toThrow('<message>')`, which Vitest
treats as a SUBSTRING match — so the rule asserting exactness was
itself inexact. Now pins with toBe on a captured message.
- The subBlock-rename qualification offered "keep the tool param name and
clear the reserved key" for a required user-only param, which cannot
satisfy both rules at once. Rename plus a migration is the only answer.
- The reserved-key checklist item was unconditional, which would forbid a
block legitimately setting the transport's timeout, proxy, or method.
Scoped to a collision where the subBlock means something provider-specific.
|
@greptile review |
|
@cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
… list Two review findings, both valid. The swallowed postProcess throw does NOT bill on Enrow. Both getCredits implementations return 0 when the output carries no `qualification`, and the fall-back submit response has none — deliberately, per the comment in verify_email.ts. The real consequence is that a stale SUCCESS reaches both the user and the pricing hook, and whether that charges depends entirely on the tool's own getCost. Restated as the rule that matters: write getCost so it cannot charge for a result the poll never produced, and do not rely on the failure propagating, because it does not. The Step 3 checklist named all three url-path helpers without the availability caveat that Step 9 and add-tools carry, so following it against staging produces an import that will not compile. Gated.
|
@greptile review |
|
@cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 4 files
Confidence score: 3/5
- In
.agents/skills/validate-integration/SKILL.md, the path-safety assertion may accept an unsafe URL builder by conflating encoded%2Fwith a real separator, weakening protection against incorrect path handling; asserturl.pathnamedirectly. - In
.agents/skills/validate-integration/SKILL.md, the fallback submit-result description incorrectly implies every field is null whileidremains populated, which could make failure-path tests encode the wrong execution contract; specify that onlyemailandqualificationare null.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".agents/skills/validate-integration/SKILL.md">
<violation number="1" location=".agents/skills/validate-integration/SKILL.md:549">
P2: This assertion cannot distinguish an encoded `%2F` from a real path separator, so a path-safety test can pass an unsafe builder. Assert `url.pathname` directly to pin the encoded output.</violation>
<violation number="2" location=".agents/skills/validate-integration/SKILL.md:578">
P3: The fallback submit result does not have every field null: `id` remains populated. Describe `email` and `qualification` as null while preserving the non-null job id, so failure-path tests do not encode the wrong executor result shape.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| equality, and pin the output with `toBe`: | ||
|
|
||
| ```typescript | ||
| expect(decodeURIComponent(url.pathname)).toBe('<the exact expected path>') |
There was a problem hiding this comment.
P2: This assertion cannot distinguish an encoded %2F from a real path separator, so a path-safety test can pass an unsafe builder. Assert url.pathname directly to pin the encoded output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .agents/skills/validate-integration/SKILL.md, line 549:
<comment>This assertion cannot distinguish an encoded `%2F` from a real path separator, so a path-safety test can pass an unsafe builder. Assert `url.pathname` directly to pin the encoded output.</comment>
<file context>
@@ -329,13 +340,265 @@ If any tool lists, searches, exports, imports, downloads, uploads, paginates, ba
+equality, and pin the output with `toBe`:
+
+```typescript
+expect(decodeURIComponent(url.pathname)).toBe('<the exact expected path>')
+
+let message = ''
</file context>
|
|
||
| `executeTool` wraps every `postProcess` call in a catch that logs and then restores the | ||
| pre-`postProcess` result (`apps/sim/tools/index.ts:1977` and `:2062`). For a submit-then-poll tool | ||
| that pre-`postProcess` result is the **submit** response — `success: true` with every result field |
There was a problem hiding this comment.
P3: The fallback submit result does not have every field null: id remains populated. Describe email and qualification as null while preserving the non-null job id, so failure-path tests do not encode the wrong executor result shape.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .agents/skills/validate-integration/SKILL.md, line 578:
<comment>The fallback submit result does not have every field null: `id` remains populated. Describe `email` and `qualification` as null while preserving the non-null job id, so failure-path tests do not encode the wrong executor result shape.</comment>
<file context>
@@ -329,13 +340,265 @@ If any tool lists, searches, exports, imports, downloads, uploads, paginates, ba
+
+`executeTool` wraps every `postProcess` call in a catch that logs and then restores the
+pre-`postProcess` result (`apps/sim/tools/index.ts:1977` and `:2062`). For a submit-then-poll tool
+that pre-`postProcess` result is the **submit** response — `success: true` with every result field
+null. So a `postProcess` that throws on a timed-out or exhausted poll is reported to the user as a
+successful lookup that simply found nothing — and that stale success is also what reaches the
</file context>
|
Closing for now — not because of a defect. This batch grew to 17 PRs across ~700 changed call sites, and we would rather revisit it as smaller, independently testable pieces than merge this much at once. Nothing here is lost: the branch |
1 participant
Eight traps found empirically across a 15-PR integration-hardening sweep, each costing real debugging and currently written down nowhere. Each learning goes into the skill that owns it, with a
file:linecitation so the guidance stays checkable; the other skills cross-reference rather than duplicate.Additive and surgical — no restructuring, one deliberate correction noted below.
add-toolsapps/sim/tools/request-transport.tsreadstimeout(:191),proxyUrl(:198), andmethod(:167) offparamsfor its own use beforerequestsees them.timeoutis the outbound HTTP deadline in milliseconds, sodaytona/execute_command.ts:49— documented as "Timeout in seconds (defaults to 10 seconds)" — aborts the call after 10ms. Still live onstaging.encodeURIComponentdoes not stop./..: both are unreserved, and the WHATWG URL parser removes dot segments after decoding. Documents when each of the threetools/url-path.tshelpers applies (safeUrlPathSegment/safeUrlPath/safeEncodedUrlPathSegment) and why onlysafeUrlPathrefuses to trim.params.x?.trim()guardsundefined, not the type. A<Block.output>resolving to a number throws a rawTypeError;toGuardedString(url-path.ts:98) is why the helpers do not.add-blocktools.config.paramsdoes NOT drop it.executor/handlers/generic/generic-handler.ts:191does{ ...inputs, ...transformedParams }, so the raw subBlock value merges back in. Clearing a reserved name needs an explicitundefined.scripts/check-block-registry.ts:181/:225enforce it, and_removed_entries insubblock-migrations.tscover genuine removals.outputsdo not drive variable resolution.executor/utils/block-reference.ts:239walks the runtime object; the schema is consulted only at:242when the value is alreadyundefined. Changing an output's shape therefore breaks saved references that were never declared — spread raw keys last.validate-integrationcatch { return }lets one guarded param hide all its siblings —x_manage_block.targetUserIdandokta_remove_user_from_group.userIdwere fully unguarded and passed everything. The sound shape: enumerate (tool, param) pairs and fuzz one at a time; assert named rejection, not path shape (shape misses%2Fand a trailing bare.); probe conditional and presence branches; assert the skipped/unbuildable ledger is empty.apps/sim/tsconfig.jsonexcludes**/*.test.tsandvitest.config.tsdeclares notypecheckblock.`${params.id.trim()}`as the path pattern are replaced — that is the exact defect this sweep fixed.add-integrationFour new entries under Common Gotchas, pointers only.
Gates
bun run lint,bun run check:audits(39 audits, includescheck:skills), andbun run skills:sync(36 skills already in sync — no projection to commit) all pass.