feat(source-greenhouse): non-breaking prep for Harvest v3 migration - #83811
Conversation
Co-Authored-By: bot_apk <apk@cognition.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksPR Slash CommandsAirbyte Maintainers (that's you!) can execute the following slash commands on your PR:
📚 Show Repo GuidanceHelpful Resources
|
|
Note ✅ All modified connector(s) now have autopilot progressive rollouts enabled. Thanks! |
|
|
Deploy preview for airbyte-docs ready!
Deployed with vercel-action |
Co-Authored-By: bot_apk <apk@cognition.ai>
|
↪️ Triggering |
|
Co-Authored-By: bot_apk <apk@cognition.ai>
|
🔀 Resolved merge conflict with Resolution: master's dependency-update release ( Pushed the resolved branch; its CI and review gates will re-run on the new HEAD. |
Co-Authored-By: bot_apk <apk@cognition.ai>
…hase1' into devin/1786404163-greenhouse-v3-phase1
|
☑️ Conflict with The conflict came from #83956 ( Since the base image moved, I also re-constructed the manifest against the CDK bundled in SDM 7.24.0: all 36 streams build, the fully-qualified |
Patrick Nilan (pnilan)
left a comment
Patrick Nilan (pnilan)
left a comment
There was a problem hiding this comment.
PR #83811 - API-Source Authoritative Review Findings
🤖 This comment was generated by an AI Agent.
Reduced-confidence review
This review ran in a degraded state. Every degradation recorded for this run, verbatim:
- no Codex reviewer returned a result: this run had NO independent second-model generation pass. Detail: completeness: NO RESULT (No result.json ever written; codex-panel-completeness-raw.txt is 0 bytes. codex-panel-completeness-result.json.detach.log (802KB) shows a terminal wrapper failure, not an in-progress run: first line 'Structured output error: Codex exited with status 1: OpenAI Codex v0.146.0', and the log ends with 'ERROR: Codex ran out of room in the model's context window. Start a new thread or clear earlier history before retrying.' (printed twice) followed by 'tokens used' / '0'. No live process matches 'codex-panel-.*-prompt' (checked via pgrep/ps and a bounded poll loop that returned NO_LIVE_RUNS immediately) - the run is dead, not still executing. Root cause: the ~801KB prompt file overflowed the model's context window before any tokens were processed.); apidoc: NO RESULT (No result.json ever written; codex-panel-apidoc-raw.txt is 0 bytes. codex-panel-apidoc-result.json.detach.log (819KB) shows the same terminal wrapper failure: first line 'Structured output error: Codex exited with status 1: OpenAI Codex v0.146.0', log ends with 'ERROR: Codex ran out of room in the model's context window. Start a new thread or clear earlier history before retrying.' (x2) then 'tokens used' / '0'. No live process found (pgrep/ps + poll loop returned NO_LIVE_RUNS). Root cause: ~818KB prompt overflowed the context window pre-processing.); cdk: NO RESULT (No result.json ever written; codex-panel-cdk-raw.txt is 0 bytes. codex-panel-cdk-result.json.detach.log (819KB) shows the same terminal wrapper failure: first line 'Structured output error: Codex exited with status 1: OpenAI Codex v0.146.0', log ends with 'ERROR: Codex ran out of room in the model's context window. Start a new thread or clear earlier history before retrying.' (x2) then 'tokens used' / '0'. No live process found (pgrep/ps + poll loop returned NO_LIVE_RUNS). Root cause: ~819KB prompt overflowed the context window pre-processing.); schema: NO RESULT (No result.json ever written; codex-panel-schema-raw.txt is 0 bytes. codex-panel-schema-result.json.detach.log (819KB) shows the same terminal wrapper failure: first line 'Structured output error: Codex exited with status 1: OpenAI Codex v0.146.0', log ends with 'ERROR: Codex ran out of room in the model's context window. Start a new thread or clear earlier history before retrying.' (x2) then 'tokens used' / '0'. No live process found (pgrep/ps + poll loop returned NO_LIVE_RUNS). Root cause: ~819KB prompt overflowed the context window pre-processing.); incremental: NO RESULT (No result.json ever written; codex-panel-incremental-raw.txt is 0 bytes. codex-panel-incremental-result.json.detach.log (819KB) shows the same terminal wrapper failure: first line 'Structured output error: Codex exited with status 1: OpenAI Codex v0.146.0', log ends with 'ERROR: Codex ran out of room in the model's context window. Start a new thread or clear earlier history before retrying.' (x2) then 'tokens used' / '0'. No live process found (pgrep/ps + poll loop returned NO_LIVE_RUNS). Root cause: ~818KB prompt overflowed the context window pre-processing.); testing: NO RESULT (No result.json ever written; codex-panel-testing-raw.txt is 0 bytes. codex-panel-testing-result.json.detach.log (799KB) shows the same terminal wrapper failure: first line 'Structured output error: Codex exited with status 1: OpenAI Codex v0.146.0', log ends with 'ERROR: Codex ran out of room in the model's context window. Start a new thread or clear earlier history before retrying.' (x2) then 'tokens used' / '0'. No live process found (pgrep/ps + poll loop returned NO_LIVE_RUNS). Root cause: ~798KB prompt overflowed the context window pre-processing.); breaking: NO RESULT (No result.json ever written; codex-panel-breaking-raw.txt is 0 bytes. codex-panel-breaking-result.json.detach.log (820KB) shows the same terminal wrapper failure: first line 'Structured output error: Codex exited with status 1: OpenAI Codex v0.146.0', log ends with 'ERROR: Codex ran out of room in the model's context window. Start a new thread or clear earlier history before retrying.' (x2) then 'tokens used' / '0'. No live process found (pgrep/ps + poll loop returned NO_LIVE_RUNS). Root cause: ~820KB prompt overflowed the context window pre-processing.)
Because of this, do not read agreement between two independent generating models into any finding below, and do not treat the absence of a finding as evidence that a dimension is clean: for all seven dimensions (completeness, api-docs, cdk-patterns, schema, incremental, testing, breaking-change) the second-model generation pass produced nothing at all rather than producing a clean bill of health. The Claude-side generation pass and both validation passes did complete, so the findings that are present were independently validated; coverage breadth, not finding quality, is what is reduced.
Review metadata
| Field | Value |
|---|---|
| PR | #83811 |
| Connector | source-greenhouse (low-code-components) |
| Head | 4fe88e9370b3f8f1ad451047ce6d720dbf0a4b85 |
| CDK (authoritative reference) | pinned 7.24.0 - the version in the connector's base image |
| CDK main (upgrade reference only) | 68bc2941768ce02b987d727e8113b14d90204684 |
| Date | 2026-08-11 |
| Provenance | Reviewed across 7 API-source dimensions by 7/7 Claude reviewers and 0/7 Codex (gpt-5.5) reviewers. CDK ground truth: the connector pinned version 7.24.0 (worktree verified); origin/main @ 68bc2941768ce02b987d727e8113b14d90204684 consulted only as an upgrade reference Third-party API docs: fetched. GitHub CI: 35 passed, 0 failed, 0 pending, 14 skipped; connector CI ran. Findings were mechanically anchored to changed lines, merged, then validated (17/17 Claude verdicts, 17/17 Codex verdicts) and reconciled. Review status: DEGRADED. Claude-side agents ran on the session model; no model version is asserted here. |
Checks
GitHub CI: 35 passed / 0 failed / 0 pending / 14 not-passed (13 skipped + 1 cancelled). No failing checks, and none pending. Connector CI did run (same-repo branch, not a gated fork): Test source-greenhouse Connector passed - 3 unit tests under the lockfile CDK 6.56.7 plus the full acceptance suite against a real built image with live Greenhouse credentials (11 passed, 3 skipped).
Skipped and cancelled checks are not passes. The ones that matter here:
| Check | Status | Note |
|---|---|---|
| Build and Verify Artifacts | SKIPPED | image build/spec/check/read was nevertheless exercised inside 'Test source-greenhouse Connector' |
| Pre-Release Checks | SKIPPED | no pre-release image was published, so no live regression / record-diff run happened |
| Connectors CDK Version Check | SKIPPED | the unit_tests CDK pin bump 6.10.0 -> 6.56.7 was never evaluated by the CDK-version gate |
| Validate PR Title | SKIPPED | bot author; 'Enforce PR structure' did pass |
| CodeQL (umbrella) | NEUTRAL/skipping | 'Analyze Python' and 'Analyze (python)' both passed |
| Call Connector CI Tests | SKIPPED | fork-gated workflow; not a gap here - same-repo branch, and connectors_ci ran the connector tests directly |
| Vercel Preview | CANCELLED | superseded by 'Vercel - airbyte-docs', which passed |
PR-specific experiments were run to produce evidence CI cannot:
| Kind | Experiment | Status | What it established |
|---|---|---|---|
| regression_control | Revert whole fix: run new test against pre-PR manifest | passed | Both new parametrized cases FAIL against the pre-PR manifest (NoMockAddress on a second-page URL carrying per_page=100 and created_after), so the new test genuinely gates the fix and fails for the exact reason the PR body claims. |
| regression_control | Surgical revert: un-gate only the request params, keep the url refactor | passed | With the url_base->url refactor left intact and only the 'if not next_page_token' guard removed, both cases still fail - isolating the failure to request-parameter gating rather than the URL restructuring. |
| coverage_completeness | Enumerate all streams in the $ref-resolved manifest | passed | 36/36 streams carry a fully-qualified url with no residual path/url_base; 30/30 paginated streams carry gated per_page and the 6 unpaginated streams carry none; 15/15 incremental streams carry exactly one gated cursor filter; zero page_size_option/start_time_option survive. 0 problems. |
| behaviour_probe | Pre/post endpoint equivalence across all 36 streams | passed | Resolved old url_base+path vs new url for every stream: 36/36 identical, 0 streams added or removed, no diff in http_method / error_handler / authenticator. Substantiates the 'same v1 endpoints' claim mechanically rather than by reading the diff. |
| version_matrix | Unit suite under base-image CDK 7.24.0 vs lockfile CDK 6.56.7 | FAILED | 3 passed under the lockfile pin 6.56.7 (what CI ran). Under 7.24.0 - the version in the connector's own base image - the two NEW tests error out: AttributeError 'DefaultStream' object has no attribute 'stream_slices' / 'retriever'. Test-harness incompatibility, not a manifest defect; this is the measured basis for F02 and F07. |
| behaviour_probe | Request-parameter interpolation under both CDK versions | passed | Byte-identical results on 6.56.7 and 7.24.0: the gated form yields page1 {per_page, cursor filter} and page2 {}; the un-gated form still carries both on page 2; the issue's 'else None' form yields per_page='None'. The manifest change is version-stable; only the test is CDK-6-bound. |
| behaviour_probe | Live request shapes emitted by the CI acceptance run (SDM 7.24.0 image) | passed | 27 distinct live first-page URLs recovered from CI's own acceptance log: every paginated stream emitted per_page=100 (plus the cursor filter where incremental), while disciplines, schools and tags/candidate emitted no query string - matching the static table. No second-page cursor URL appears anywhere in the log, so live evidence covers first-page shape only. |
| fallback_suite | Run connector unit tests as CI ground-truth substitute | not run | Not applicable - CI was available and connector CI ran (3 unit tests, plus the full acceptance suite with real secrets), so there was no ground truth to recover. |
Coverage caveat. CI built the connector image and ran spec/check/discover/read against live Greenhouse Harvest v1 with real credentials (11 passed, 3 skipped) plus 3 unit tests under the lockfile CDK 6.56.7, but no live regression/record-diff against the current release was performed (Pre-Release Checks skipped, no pre-release image), and the new mocked request-shape test cannot execute at all under CDK 7.24.0 - the version in the connector's own base image - so its assertions only cover the CDK-6 code path.
Summary
| Severity | Authoritative findings |
|---|---|
| 🔴 P0 | 0 |
| 🟠 P1 | 0 |
| 🟡 P2 | 3 |
| 🔵 P3 | 5 |
| ⚪ P4 | 3 |
| Total | 11 |
Verdict: Fix before merge. No authoritative P0 and no unversioned breaking change, but three P2 findings should be resolved before merge.
6 candidate findings were dropped during validation (0 unvalidated); their ids, both reviewer verdicts and the reasoning are in pr-83811-api-source-authoritative-findings-appendix.md, together with the full breaking-change evaluation and the complete run-coverage record.
Breaking change: No
Findings at a glance
- 🟡 F01 - Cursor filter dropped after page 1 — On incremental syncs the "only give me records changed since X" filter is applied to the first page of results only, and it was never established that Greenhouse carries that filter into the follow-on pages. On any of the 15 incremental streams with more than one page of changes, a sync could quietly return unrelated older records instead of the changed ones, report success, and advance its bookmark past the records it never fetched.
- 🟡 F02 - Regression test cannot run on the shipped runtime — The single test written to protect this change only runs against an older version of the connector framework; on the version the connector actually ships with it errors out before making a single assertion. The behaviour change therefore has no working safety net on the runtime customers run.
- 🟡 F07 - Test framework pin a major version behind what ships — The test suite's framework version was raised, but it stops a full major version short of the version the connector actually runs on, so the tests validate code that differs from what ships in exactly the area this PR rewrote. The automated check that would have flagged the mismatch did not run on this PR.
- 🔵 F03 - Filter duplicated 15 times instead of derived from the cursor — The "changed since" filter is now written out by hand fifteen times instead of being derived from the single place that defines the sync bookmark. When the planned next phase changes the bookmark's format, those fifteen copies will silently fall out of step and the affected streams will quietly revert to pulling full history rather than failing.
- 🔵 F04 - Dead page-size setting left in 30 places — A page-size setting is still repeated 30 times across the connector but nothing reads it any more. The next phase of this migration plans to raise the page size by editing exactly that setting, which would have no effect at all and produce no warning or test failure.
- 🔵 F05 - Spec description still quotes the old API's rate limit — The user-facing description of the worker-count setting still quotes a rate limit that belongs to the old Greenhouse API version, even though this PR was supposed to remove that hard-coded number and already repointed the connector's rate-limit metadata link at the new API's policy.
- 🔵 F09 - Sub-stream test bypasses the real bookmark — The test case for streams that read records per parent record (for example interviews for each application) skips the real bookmark machinery and hands the test a made-up value instead. The part most likely to break silently - and turn those streams into full-history scans - is the part not being checked.
- 🔵 F11 - Docs link to the new API while the connector emits old-API fields — 32 stream reference links in the docs now point at Greenhouse's new API documentation, but the connector still reads the old API and still emits the old field names. A user mapping destination columns from those pages will look for fields the connector never produces - including one field that is a text status in the old API and a true/false flag in the new one.
- ⚪ F12 - Four doc links left on the retiring site — The documentation link migration was only partly applied. Activity Feed, Degrees, Disciplines and Schools still link to the Greenhouse documentation site that is being retired at the end of this month, while the stream entries listed immediately next to them were updated.
- ⚪ F15 - Test mutates global module state with no teardown — The new test registers a module globally and never removes it, so it changes how every test that runs after it in the same session resolves the connector's custom code. No harm today, but it is a latent cross-test contamination source.
- ⚪ F17 - Changelog date inversion — The changelog row for the new release is dated one day earlier than the release directly beneath it, so the version history reads as though the newer release shipped first.
Detailed findings & prescriptive fixes (for the PR author)
F01 - Incremental cursor filter is suppressed on every page after the first, and nothing establishes that Harvest v1's Link: rel="next" URL carries the filter forward (P2)
Location: airbyte-integrations/connectors/source-greenhouse/manifest.yaml:25 (and the identical line in the other 14 incremental streams: 79, 142, 202, 373, 680, 779, 830, 881, 935, 995, 1086, 1177, 1366, 1417)
Reviewer verdicts: Claude - valid (P2) / Codex - valid (P2)
Why it matters: The if not next_page_token gate is required only by Harvest v3's cursor-exclusivity rule, but Phase 1 still calls v1, whose docs never guarantee the rel="next" URL echoes created_after/updated_after/submitted_after. Gating it now buys nothing (Phase 2 renames these params to v3's created_at=gte|... form anyway) and risks page 2+ walking the unfiltered collection.
Prescriptive fix
airbyte-integrations/connectors/source-greenhouse/manifest.yaml:25 (and the identical line in the other 14 incremental streams: 79, 142, 202, 373, 680, 779, 830, 881, 935, 995, 1086, 1177, 1366 submitted_after, 1417).
Why it matters: the if not next_page_token gate is required only by Harvest v3's cursor-exclusivity rule, but Phase 1 still calls v1, whose docs never guarantee the rel="next" URL echoes created_after/updated_after/submitted_after. Gating it now buys nothing (Phase 2 renames these params to v3's created_at=gte|... form anyway) and risks page 2+ walking the unfiltered collection. Leave the per_page gate alone - it is provably safe, since v1 echoes per_page into the next link and 100 is v1's documented default.
Before (git show 4fe88e9370b3f8f1ad451047ce6d720dbf0a4b85:airbyte-integrations/connectors/source-greenhouse/manifest.yaml, lines 23-25):
request_parameters:
per_page: "{{ 100 if not next_page_token }}"
created_after: "{{ stream_interval.start_time if not next_page_token }}"After:
request_parameters:
per_page: "{{ 100 if not next_page_token }}"
created_after: "{{ stream_interval.start_time }}"This restores 0.7.32's exact wire shape for the filter: SimpleRetriever._request_params passes stream_slice on every page, so stream_interval.start_time still resolves on page 2+, and if the v1 Link URL does echo the filter, HttpClient._dedupe_query_params drops the duplicate - correct under both possible v1 behaviours. (Rejected alternative: restoring the deleted start_time_option blocks - same wire result but a 15-hunk revert instead of a 15-line edit.)
The new test must be updated to match, otherwise it fails: HttpRequest.matches compares parse_qs of the full query exactly, and unit_tests/test_pagination_request_params.py:71 currently asserts the bare link URL. Change
second_request = HttpRequest(next_url)to
second_request = HttpRequest(f"{next_url}&{cursor_parameter}={cursor_value}")(the mocked next_url should stay param-free/v3-shaped rather than being made "realistic" - see the audit appendix entry for F08, where a v1-shaped link was measured to silently disable the per_page guard.)
RECONCILIATION NOTE: if F03's fix is preferred instead, it must be applied in the form that restores start_time_option WITHOUT ignore_stream_slicer_parameters_on_paginated_requests - setting that flag reproduces exactly the page-2 suppression this finding objects to. See F03.
F02 - The only regression test for this change is written against the legacy CDK 6 stream API and cannot execute on the CDK the connector ships on (P2)
Location: airbyte-integrations/connectors/source-greenhouse/unit_tests/test_pagination_request_params.py:37-100
Reviewer verdicts: Claude - valid (P2, cdk_version_mismatch) / Codex - valid (P2, cdk_version_mismatch)
Why it matters: This is the only regression test for the PR's central change, and it cannot execute on source-declarative-manifest:7.24.0 - verified AttributeError: 'DefaultStream' object has no attribute 'stream_slices'. Driving source.read() with a ConfiguredAirbyteCatalog uses the one entry point that is stable across 6.56.7 and 7.24.0, so the test both survives the pin bump and asserts the request construction that actually ships.
Prescriptive fix
File: airbyte-integrations/connectors/source-greenhouse/unit_tests/test_pagination_request_params.py:37-100 (rewrite the test body; the parametrize block at L16-36 is replaced too).
Why it matters: this is the only regression test for the PR's central change, and it cannot execute on source-declarative-manifest:7.24.0 - verified AttributeError: 'DefaultStream' object has no attribute 'stream_slices'. Driving source.read() with a ConfiguredAirbyteCatalog uses the one entry point that is stable across 6.56.7 and 7.24.0, so the test both survives the pin bump and asserts the request construction that actually ships.
BEFORE (actual head content, git show 4fe88e9...:.../test_pagination_request_params.py, L47-100 shown):
sys.modules["components"] = components_module
source = YamlDeclarativeSource(str(manifest_path), config={"api_key": "test"})
stream = next(stream for stream in source.streams(config={"api_key": "test"}) if stream.name == stream_name)
stream_slice = (
next(
iter(
stream.stream_slices(
sync_mode=SyncMode.incremental,
cursor_field=[cursor_field],
stream_state={},
)
)
)
if not partition
else StreamSlice(
partition=partition,
cursor_slice={
"start_time": cursor_value,
"end_time": "2024-01-03T03:04:05.678Z",
},
)
)
next_url = f"https://harvest.greenhouse.io/v1/{path}?cursor=ABC&foo=bar"
first_request = HttpRequest(
f"https://harvest.greenhouse.io/v1/{path}",
query_params={
"per_page": "100",
cursor_parameter: cursor_value,
},
)
second_request = HttpRequest(next_url)
cursor_context = patch.object(stream.retriever, "cursor", None) if partition else nullcontext()
with cursor_context:
with HttpMocker() as http_mocker:
http_mocker.get(
first_request,
HttpResponse(
json.dumps([{"id": 1, cursor_field: cursor_value}]),
headers={"Link": f'<{next_url}>; rel="next"'},
),
)
http_mocker.get(
second_request,
HttpResponse(json.dumps([{"id": 2, cursor_field: "2024-01-03T03:04:05.678Z"}])),
)
records = list(
stream.read_records(
sync_mode=SyncMode.incremental,
stream_slice=stream_slice,
)
)
http_mocker.assert_number_of_calls(first_request, 1)
http_mocker.assert_number_of_calls(second_request, 1)AFTER (whole file; verified 2 passed on airbyte-cdk 6.56.7 AND on 7.24.0):
# Copyright (c) 2026 Airbyte, Inc., all rights reserved.
import json
import logging
import sys
import pytest
from airbyte_cdk.models import (
AirbyteStream,
ConfiguredAirbyteCatalog,
ConfiguredAirbyteStream,
DestinationSyncMode,
SyncMode,
)
from airbyte_cdk.sources.declarative.yaml_declarative_source import YamlDeclarativeSource
from airbyte_cdk.test.mock_http import HttpMocker, HttpRequest, HttpResponse
_CONFIG = {"api_key": "test"}
_PARENT_URL = "https://harvest.greenhouse.io/v1/applications"
_START = "1970-01-01T00:00:00.000Z"
def _catalog(stream_name: str, cursor_field: str) -> ConfiguredAirbyteCatalog:
return ConfiguredAirbyteCatalog(
streams=[
ConfiguredAirbyteStream(
stream=AirbyteStream(
name=stream_name,
json_schema={},
supported_sync_modes=[SyncMode.incremental],
),
sync_mode=SyncMode.incremental,
destination_sync_mode=DestinationSyncMode.append,
cursor_field=[cursor_field],
)
]
)
@pytest.mark.parametrize(
("stream_name", "path", "cursor_field", "cursor_parameter", "needs_parent"),
[
("applications", "applications", "applied_at", "created_after", False),
(
"applications_interviews",
"applications/123/scheduled_interviews",
"updated_at",
"updated_after",
True,
),
],
)
def test_incremental_pagination_request_parameters(
manifest_path,
components_module,
stream_name,
path,
cursor_field,
cursor_parameter,
needs_parent,
):
sys.modules["components"] = components_module
catalog = _catalog(stream_name, cursor_field)
source = YamlDeclarativeSource(str(manifest_path), config=_CONFIG, catalog=catalog, state=[])
next_url = f"https://harvest.greenhouse.io/v1/{path}?cursor=ABC&foo=bar"
first_request = HttpRequest(
f"https://harvest.greenhouse.io/v1/{path}",
query_params={"per_page": "100", cursor_parameter: _START},
)
second_request = HttpRequest(next_url)
with HttpMocker() as http_mocker:
if needs_parent:
http_mocker.get(
HttpRequest(
_PARENT_URL,
query_params={"per_page": "100", "created_after": _START},
),
HttpResponse(json.dumps([{"id": 123, "applied_at": _START}])),
)
http_mocker.get(
first_request,
HttpResponse(
json.dumps([{"id": 1, cursor_field: _START}]),
headers={"Link": f'<{next_url}>; rel="next"'},
),
)
http_mocker.get(
second_request,
HttpResponse(json.dumps([{"id": 2, cursor_field: "2024-01-03T03:04:05.678Z"}])),
)
records = [
message.record.data
for message in source.read(logging.getLogger("test"), _CONFIG, catalog, [])
if message.type.value == "RECORD" and message.record.stream == stream_name
]
http_mocker.assert_number_of_calls(first_request, 1)
http_mocker.assert_number_of_calls(second_request, 1)
assert records == [
{"id": 1, cursor_field: _START},
{"id": 2, cursor_field: "2024-01-03T03:04:05.678Z"},
]Notes on the delta: contextlib.nullcontext, unittest.mock.patch and airbyte_cdk.sources.types.StreamSlice imports (L5-6, L12) become unused and must be dropped - the patch.object(stream.retriever, "cursor", None) hack at L79 exists only to defeat the substream cursor and is unnecessary once the real read path runs with the parent mocked. The substream case now starts at the manifest's start_datetime of 1970-01-01T00:00:00.000Z instead of the hand-built 2024-01-02... slice, because the cursor is derived rather than fabricated.
RECONCILIATION NOTE: this rewrite and F09's generate_partitions() rewrite are alternatives, not both. Land F07's pin bump to 7.24.0 in the same commit and prefer F09's version, which was measured to FAIL on two injected regressions (un-gated per_page, and partition_field_start: since); this source.read() version is the fallback if the pin has to stay at 6.56.7. Whichever is chosen, apply F01's second_request change on top if F01's fix lands.
F07 - Unit-test CDK pin bumped to 6.56.7 while the connector runs on source-declarative-manifest:7.24.0, and the CDK Version Check gate was skipped (P2)
Location: airbyte-integrations/connectors/source-greenhouse/unit_tests/pyproject.toml:13
Reviewer verdicts: Claude - valid (P2, cdk_version_mismatch) / Codex - valid (P2, cdk_version_mismatch)
Why it matters: The suite currently validates a CDK a full major behind what ships, and the two differ in exactly the RequestPath/url code path this PR rewrote - so the new regression test cannot protect the change on the runtime that runs it.
Prescriptive fix
airbyte-integrations/connectors/source-greenhouse/unit_tests/pyproject.toml:13.
Why it matters: the suite currently validates a CDK a full major behind what ships, and the two differ in exactly the RequestPath/url code path this PR rewrote - so the new regression test cannot protect the change on the runtime that runs it.
BEFORE (unit_tests/pyproject.toml:11-15):
[tool.poetry.dependencies]
python = "^3.10,<3.13"
airbyte-cdk = "6.56.7"
pytest = "^8"
requests-mock = "^1.12.1"AFTER:
[tool.poetry.dependencies]
python = "^3.10,<3.13"
airbyte-cdk = "7.24.0"
pytest = "^8"
requests-mock = "^1.12.1"Then regenerate the lockfile: cd airbyte-integrations/connectors/source-greenhouse/unit_tests && poetry lock.
This must land in the same commit as the test port in F09 - with the pin at 7.24.0 the current test errors out ('DefaultStream' object has no attribute 'stream_slices'), while the F09 replacement passes 2/2 at 7.24.0. (Rejected alternative: leaving the pin at 6.56.7 and adding a comment - that keeps CI green while testing code that does not ship.)
F03 - Cursor filter is hand-rolled into 15 duplicated Jinja templates instead of using the CDK's built-in ignore_stream_slicer_parameters_on_paginated_requests, decoupling the filter from the cursor that owns it (P3)
Location: airbyte-integrations/connectors/source-greenhouse/manifest.yaml:17-25 and :55-58 (and the equivalent pair in each of the other 14 incremental streams)
Reviewer verdicts: Claude - valid (P2) / Codex - overly_defensive (P4)
Why it matters: The filter value is currently a free-text Jinja literal duplicated 15 times and no longer derived from the cursor that owns it, so Phase 2's gte| prefix / format change applied to datetime_format or partition_field_start will silently diverge from these templates and degrade to an unfiltered full pull with no error. The pinned CDK already supplies the exact suppression this PR hand-rolls.
Prescriptive fix
RECONCILIATION NOTE - READ FIRST: this fix and F01's fix produce OPPOSITE page-2 wire behaviour. Setting ignore_stream_slicer_parameters_on_paginated_requests: true keeps the cursor filter suppressed on page 2+, which is exactly what F01 (P2, higher severity) objects to. If F01's fix is adopted, apply only the start_time_option half below and do NOT set the flag: that single form satisfies both findings - the filter is sent on every page (F01) and its value is bound back to the cursor with typed field_name/inject_into instead of 15 duplicated literals (F03).
airbyte-integrations/connectors/source-greenhouse/manifest.yaml:17-25 and :55-58 (and the equivalent pair in each of the other 14 incremental streams).
Why it matters: the filter value is currently a free-text Jinja literal duplicated 15 times and no longer derived from the cursor that owns it, so Phase 2's gte| prefix / format change applied to datetime_format or partition_field_start will silently diverge from these templates and degrade to an unfiltered full pull with no error. The pinned CDK already supplies the exact suppression this PR hand-rolls.
BEFORE (manifest.yaml:17-25, applications):
retriever:
type: SimpleRetriever
requester:
$ref: "#/definitions/base_requester"
url: https://harvest.greenhouse.io/v1/applications
http_method: GET
request_parameters:
per_page: "{{ 100 if not next_page_token }}"
created_after: "{{ stream_interval.start_time if not next_page_token }}"AFTER:
retriever:
type: SimpleRetriever
ignore_stream_slicer_parameters_on_paginated_requests: true
requester:
$ref: "#/definitions/base_requester"
url: https://harvest.greenhouse.io/v1/applications
http_method: GET
request_parameters:
per_page: "{{ 100 if not next_page_token }}"BEFORE (manifest.yaml:55-58, same stream):
start_datetime:
type: MinMaxDatetime
datetime: "1970-01-01T00:00:00.000Z"
datetime_format: "%Y-%m-%dT%H:%M:%S.%_msZ"AFTER:
start_datetime:
type: MinMaxDatetime
datetime: "1970-01-01T00:00:00.000Z"
datetime_format: "%Y-%m-%dT%H:%M:%S.%_msZ"
start_time_option:
type: RequestOption
field_name: created_after
inject_into: request_parameterApply the same two edits to the other 14 incremental streams, restoring each stream's own field_name (updated_after for 13 of them, submitted_after for eeoc). Keep the hand-rolled per_page exactly as-is - the flag deliberately does not suppress page_size_option, and the pinned schema states plainly that "Request options set directly on the requester will not be ignored" (declarative_component_schema.yaml:4175-4177). (Rejected alternative: leaving the templates and adding a comment documenting the coupling - that keeps 15 duplicated strings the schema cannot validate.)
F04 - Removing page_size_option leaves page_size: 100 on all 30 CursorPagination strategies as dead config, a trap for the Phase 2 page-size raise (P3)
Location: airbyte-integrations/connectors/source-greenhouse/manifest.yaml:44 (and the other 29 identical lines under each CursorPagination block)
Reviewer verdicts: Claude - valid (P3) / Codex - valid (P3)
Why it matters: With page_size_option gone nothing reads page_size, and DefaultPaginator.__post_init__ only validates the reverse direction - so the Phase-2 edit page_size: 100 -> 500 will change nothing, raise nothing, and fail no test.
Prescriptive fix
airbyte-integrations/connectors/source-greenhouse/manifest.yaml:44 (and the other 29 identical lines under each CursorPagination block).
Why it matters: with page_size_option gone nothing reads page_size, and DefaultPaginator.__post_init__ only validates the reverse direction - so the Phase-2 edit page_size: 100 -> 500 will change nothing, raise nothing, and fail no test.
BEFORE (manifest.yaml:42-46):
pagination_strategy:
type: CursorPagination
page_size: 100
cursor_value: "{{ headers['link']['next']['url'] }}"
stop_condition: "{{ 'next' not in headers['link'] }}"AFTER:
pagination_strategy:
type: CursorPagination
cursor_value: "{{ headers['link']['next']['url'] }}"
stop_condition: "{{ 'next' not in headers['link'] }}"Mechanically: grep -v '^ page_size: 100$' manifest.yaml removes exactly the 30 orphaned lines. The effective page size stays the per_page: "{{ 100 if not next_page_token }}" literal in request_parameters, which is the single place Phase 2 must edit to reach v3's max of 500. (Rejected alternative: keeping page_size with an explanatory comment - a comment does not stop the wrong line from being edited.)
F05 - Issue task (3) not done: the num_workers spec description still hardcodes v1's "50 requests per 10 seconds" and the rate-limit comment still cites the retiring v1 doc URL (P3)
Location: airbyte-integrations/connectors/source-greenhouse/manifest.yaml:1731-1735 (and the comment at :1689-1691)
Reviewer verdicts: Claude - valid (P3) / Codex - valid (P3)
Why it matters: The spec description this PR claims to have reworded still quotes a v1-only number to every user, and that number stops being true the moment Phase 2 repoints the endpoints - while the metadata.yaml rate_limits link this PR did change already points at the v3 policy.
Prescriptive fix
airbyte-integrations/connectors/source-greenhouse/manifest.yaml:1731-1735 (and the comment at :1689-1691).
Why it matters: the spec description this PR claims to have reworded still quotes a v1-only number to every user, and that number stops being true the moment Phase 2 repoints the endpoints - while the metadata.yaml rate_limits link this PR did change already points at the v3 policy.
BEFORE (manifest.yaml:1731-1735):
description: >-
The number of worker threads to use for syncing. The default is tuned
against Greenhouse's documented limit of 50 requests per 10 seconds.
Increase this only if your Greenhouse account can support higher API
usage, or lower it if you see rate-limit errors.AFTER:
description: >-
The number of worker threads to use for syncing. The default is tuned
against Greenhouse's documented Harvest API rate limit. Increase this
only if your Greenhouse account can support higher API usage, or lower
it if you see rate-limit errors.And in the same edit, mark the retained v1 budget comment as version-specific rather than repointing it (the 50/PT10S budget is correct for v1 and deliberately unchanged in Phase 1):
BEFORE (manifest.yaml:1689-1691):
# Greenhouse Harvest API documented rate limit (single tier - Path A):
# - 50 requests per 10 seconds (5 req/sec) for all accounts.
# - Source: https://developers.greenhouse.io/harvest.html#throttlingAFTER:
# Harvest v1 documented rate limit (single tier - Path A). Phase 1 still calls v1,
# so api_budget below intentionally stays on the v1 ceiling:
# - 50 requests per 10 seconds (5 req/sec) for all accounts.
# - Source: https://developers.greenhouse.io/harvest.html#throttling (site retires 2026-08-31)
# Phase 2 retunes to v3's 30-second fixed window:
# https://harvestdocs.greenhouse.io/docs/api-rate-limitingF09 - The substream test case patches the retriever cursor to None and hand-builds the StreamSlice, so the one construct most likely to fail silently is the one bypassed (P3)
Location: airbyte-integrations/connectors/source-greenhouse/unit_tests/test_pagination_request_params.py:50-68 and :79 (plus :93-98)
Reviewer verdicts: Claude - valid (P3) / Codex - valid (P3)
Why it matters: The one construct most likely to break silently - a per-partition incremental slice feeding {{ stream_interval.start_time }} - is the one the test bypasses; it was verified that adding partition_field_start: since to applications_interviews keeps the current test green while silently dropping the filter.
Prescriptive fix
airbyte-integrations/connectors/source-greenhouse/unit_tests/test_pagination_request_params.py:50-68 and :79 (plus :93-98).
Why it matters: the one construct most likely to break silently - a per-partition incremental slice feeding {{ stream_interval.start_time }} - is the one the test bypasses; it was verified that adding partition_field_start: since to applications_interviews keeps the current test green while silently dropping the filter.
BEFORE (lines 50-68, then 79, then 93-98):
stream_slice = (
next(
iter(
stream.stream_slices(
sync_mode=SyncMode.incremental,
cursor_field=[cursor_field],
stream_state={},
)
)
)
if not partition
else StreamSlice(
partition=partition,
cursor_slice={
"start_time": cursor_value,
"end_time": "2024-01-03T03:04:05.678Z",
},
)
)
...
cursor_context = patch.object(stream.retriever, "cursor", None) if partition else nullcontext()
...
records = list(
stream.read_records(
sync_mode=SyncMode.incremental,
stream_slice=stream_slice,
)
)AFTER - drop the slice construction and the cursor patch entirely, mock the parent request, and drive the real cursor via generate_partitions():
parent_request = HttpRequest(
"https://harvest.greenhouse.io/v1/applications",
query_params={"per_page": "100", "created_after": cursor_value},
)
...
with HttpMocker() as http_mocker:
if stream_name != "applications":
http_mocker.get(
parent_request,
HttpResponse(json.dumps([{"id": 123, "applied_at": cursor_value}])),
)
http_mocker.get(first_request, HttpResponse(..., headers={"Link": f'<{next_url}>; rel="next"'}))
http_mocker.get(second_request, HttpResponse(...))
records = [r for p in stream.generate_partitions() for r in p.read()]
http_mocker.assert_number_of_calls(first_request, 1)
http_mocker.assert_number_of_calls(second_request, 1)
assert [dict(r.data) for r in records] == [...]The partition and cursor_value parametrize columns collapse to a single cursor_value = "1970-01-01T00:00:00.000Z" (both streams start from the manifest default), and the unittest.mock.patch, nullcontext, StreamSlice and SyncMode imports become unused. This exact shape was run under CDK 7.24.0: 2 passed on the head manifest, and it fails on both injected regressions (un-gated per_page, and partition_field_start: since). It therefore doubles as the port F07's pin bump requires. (Rejected alternative: keeping the hand-built slice and merely asserting its keys - that still never exercises the cursor that produces it.)
RECONCILIATION NOTE: generate_partitions() is 7.x-only, so this rewrite requires F07's pin bump to 7.24.0 in the same commit. Land F02/F07/F09 as one change and use this version rather than F02's source.read() variant - only this one was measured to fail on injected regressions.
F11 - 32 stream reference links repointed to Harvest v3 doc pages while all 36 streams still call v1, and several distinct streams collapse onto identical v3 URLs (P3)
Location: docs/integrations/sources/greenhouse.md:27
Reviewer verdicts: Claude - valid (P3) / Codex - valid (P3)
Why it matters: The 32 forward-looking v3 links otherwise read as a description of the records this connector currently emits, while the emitted schemas were verified byte-identical to the pre-PR (v1-shaped) versions.
Prescriptive fix
docs/integrations/sources/greenhouse.md:27 - insert a version-mismatch admonition immediately under the ## Supported Streams heading so the 32 forward-looking v3 links do not read as a description of the records this connector currently emits.
BEFORE (head, lines 27-30):
## Supported Streams
- [Activity Feed](https://developers.greenhouse.io/harvest.html#get-retrieve-activity-feed)
- [Applications](https://harvestdocs.greenhouse.io/reference/get_v3-applications) \(Incremental\)AFTER:
## Supported Streams
:::note
The reference links below point to the Greenhouse **Harvest v3** documentation, which is where these endpoints are moving. This connector still reads **Harvest v1**, so the records it emits keep the v1 field names: `applications.applied_at` (v3 renames it to `created_at`), `users.disabled` (v3: `deactivated`), `job_openings.status` (v3: `open`, a boolean instead of a string). When mapping destination columns, use the stream schema shown in the Airbyte UI rather than the linked v3 page.
:::
- [Activity Feed](https://developers.greenhouse.io/harvest.html#get-retrieve-activity-feed)
- [Applications](https://harvestdocs.greenhouse.io/reference/get_v3-applications) \(Incremental\)(Rejected alternative: reverting the 32 links to developers.greenhouse.io - that host is removed on 2026-08-31, three weeks out, so it trades a wrong description for 32 dead links.) :::note is the Docusaurus admonition already used across docs/integrations/sources/ (e.g. stripe.md, salesforce.md).
Scope note: the "five pairs collapse onto identical v3 URLs" half of this finding is NOT part of the fix - the vendor's own migration map sends /v1/scheduled_interviews and /v1/applications/{id}/scheduled_interviews both to /v3/interviews, and /v1/job_stages and /v1/jobs/{id}/stages both to /v3/job_interview_stages, so there is no distinct v3 page to point at.
F12 - Doc-link migration applied inconsistently: four stream links still cite the retiring v1 doc site (P4)
Location: docs/integrations/sources/greenhouse.md:29, :43, :45, :58
Reviewer verdicts: Claude - valid (P4) / Codex - valid (P4)
Why it matters: The page currently cites two doc sites for the same connector, and the four rows left on developers.greenhouse.io die when that host is removed on 2026-08-31.
Prescriptive fix
docs/integrations/sources/greenhouse.md lines 29, 43, 45, 58 - repoint the four skipped stream links to the v3 pages the vendor's migration guide names, so the page cites one doc site and no row dies when developers.greenhouse.io is removed on 2026-08-31.
BEFORE (head):
- [Activity Feed](https://developers.greenhouse.io/harvest.html#get-retrieve-activity-feed)
...
- [Degrees](https://developers.greenhouse.io/harvest.html#get-list-degrees)
...
- [Disciplines](https://developers.greenhouse.io/harvest.html#get-list-approvals-for-job)
...
- [Schools](https://developers.greenhouse.io/harvest.html#get-list-schools)AFTER:
- [Activity Feed](https://harvestdocs.greenhouse.io/reference/get_v3-notes)
...
- [Degrees](https://harvestdocs.greenhouse.io/reference/get_v3-custom-field-options)
...
- [Disciplines](https://harvestdocs.greenhouse.io/reference/get_v3-custom-field-options)
...
- [Schools](https://harvestdocs.greenhouse.io/reference/get_v3-custom-field-options)Leave lines 7 and 15 (the API-key setup links) and manifest.yaml:1691 on the v1 host - both are correct for a connector that still speaks v1, and the linked issue explicitly requires lines 7 and 15 to stay on v1 until Phase 2 changes auth. The Disciplines row also had the wrong v1 anchor (#get-list-approvals-for-job), which this edit incidentally retires. (Rejected alternative: leaving all four as-is until Phase 2 - that forces a second docs-only pass for four lines.)
Anchor correction: the finding's metadata.yaml:61 anchor is mis-filed - that line already reads https://harvestdocs.greenhouse.io/docs/authentication. Nothing in metadata.yaml needs to change for this finding.
F15 - sys.modules["components"] is mutated globally in the test body with no teardown (P4)
Location: airbyte-integrations/connectors/source-greenhouse/unit_tests/test_pagination_request_params.py:37-48
Reviewer verdicts: Claude - valid (P4) / Codex - valid (P4)
Why it matters: The bare sys.modules write persists past this test and silently changes how every later test in the session resolves custom components; pytest's built-in monkeypatch reverts it at teardown.
Prescriptive fix
airbyte-integrations/connectors/source-greenhouse/unit_tests/test_pagination_request_params.py:37-48 - the bare sys.modules write persists past this test and silently changes how every later test in the session resolves custom components; pytest's built-in monkeypatch reverts it at teardown.
BEFORE (head, lines 37-48):
def test_incremental_pagination_request_parameters(
manifest_path,
components_module,
stream_name,
path,
partition,
cursor_field,
cursor_parameter,
cursor_value,
):
sys.modules["components"] = components_module
source = YamlDeclarativeSource(str(manifest_path), config={"api_key": "test"})AFTER:
def test_incremental_pagination_request_parameters(
monkeypatch,
manifest_path,
components_module,
stream_name,
path,
partition,
cursor_field,
cursor_parameter,
cursor_value,
):
monkeypatch.setitem(sys.modules, "components", components_module)
source = YamlDeclarativeSource(str(manifest_path), config={"api_key": "test"})monkeypatch is a pytest builtin (function-scoped, so it composes fine with the session-scoped components_module fixture) - no new dependency and no change to the pinned CDK contract. Do NOT add a components_module is None guard: components.py exists at head and the manifest's state_migrations require it, so the fixture cannot return None - that half of the finding is dropped. (Rejected alternative: an autouse fixture wrapping the assignment - more code for a single call site.) Carry this one-line change into whichever test rewrite lands from F02/F09.
F17 - Changelog date inversion: 0.8.0 dated 2026-08-10 sits above 0.7.33 dated 2026-08-11 (P4)
Location: docs/integrations/sources/greenhouse.md:81
Reviewer verdicts: Claude - valid (P4) / Codex - valid (P4)
Why it matters: The new release must not be dated before the release directly beneath it in a newest-first table.
Prescriptive fix
docs/integrations/sources/greenhouse.md:81 - the new release must not be dated before the release directly beneath it in a newest-first table.
BEFORE (head, line 81):
| 0.8.0 | 2026-08-10 | [83811](https://github.com/airbytehq/airbyte/pull/83811) | Send pagination and incremental filter parameters only on first-page requests, and use fully-qualified per-stream URLs, in preparation for the Harvest v3 migration. |AFTER:
| 0.8.0 | 2026-08-11 | [83811](https://github.com/airbytehq/airbyte/pull/83811) | Send pagination and incremental filter parameters only on first-page requests, and use fully-qualified per-stream URLs, in preparation for the Harvest v3 migration. |If the merge slips past 2026-08-11, use the actual merge date instead - the invariant is simply that it is >= the 2026-08-11 date on the 0.7.33 row at line 82.
Co-Authored-By: bot_apk <apk@cognition.ai>
|
Thanks — useful review, especially the CDK-version experiment. Triage of the 11 findings, pushed in Fixed
Escalated, not changed: F01. This one is a real question rather than a nit, but the gate you're asking me to remove is the explicit requirement of the originating issue (airbytehq/airbyte-internal-issues#16900 task 2: drop Not changing (with reasons)
Also worth carrying forward for whoever reviews: Pre-Release Checks were skipped, so no live regression / record-diff ran, and I had no Greenhouse credentials — that acceptance criterion from the issue is still unmet, as stated in the PR description. |
|
I want you to independently verify F01 with Greenhouse's Harvest API documentation |
|
🙋 Human Input Needed (re: your request to independently verify F01) — the documentation does not answer it directly, and I want your call before changing the gate. Verdict: F01's failure mode is very unlikely to be real on v1, but the evidence for that is inference from documented pagination semantics, not an explicit vendor statement. What the docs actually say
Why the failure mode is nonetheless unlikely
What would settle it, and what I cannot do: one authenticated two-page v1 request against any incremental endpoint, e.g. Recommendation. Un-gate the incremental filter for Phase 1 (F01's fix), keeping Two notes if you take that route: it reverses task 2 of airbytehq/airbyte-internal-issues#16900 as written, so it is your call rather than mine; and it is best implemented as F03's |
|
Good callout, drop F01 as necessary. |
|
/ai-prove-fix
|
|
👍 On it. Reading "drop F01 as necessary" as: drop the gate on the incremental cursor filter, per the recommendation above. Implementing it as the typed |
|
|
🟢 Fix Proven —
|
| page 1 | page 2 (Link-derived) |
|
|---|---|---|
0.8.0 |
per_page=100 + cursor filter |
cursor filter only — no per_page |
0.7.33 |
per_page=100 + cursor filter |
cursor filter + per_page=100 |
Both versions walk to page 2 and emit every record from both pages, against the same host and path. So the v3-blocking parameter is removed from exactly the requests that would have failed, the cursor filter is untouched (the F01 decision landed correctly), and gating per_page does not truncate the page walk — the Link URL carries the page size itself.
2. Live regression comparison (Tier-2 customer connection, cold read, latest (auto) = 0.7.33 as control): every command ✅ on both versions, and every comparison check ✅ — Spec compatibility (one compatible change: the intended num_workers.description reword), Catalog schema unchanged across all 36 streams, Stream coverage, record counts, PK presence, PK integrity on both sides, field values, and Final state per stream.
| Stream | Control | Target | Delta |
|---|---|---|---|
jobs |
830 | 830 | 0 |
offers |
779 | 779 | 0 |
users |
583 | 583 | 0 |
| Total | 2,192 | 2,192 | 0 |
Every one of those streams exceeded the 100-record first page, so each crossed several page boundaries under the new gating and still returned the complete record set with identical PKs, identical field values and an identical state shape.
Next steps
- Ready to merge from a regression standpoint. Worth running
/ai-canary-prereleaseafter merge to watch organic customer syncs on the real published version before Phase 2 builds on top of it. - Phase 2 (
airbytehq/airbyte-internal-issues#16901) is the breaking half — schema and auth — and is where theapi_budgetretune to v3's window belongs; nothing in this PR pre-empts it. - Daily hands-free triage will keep watching
source-greenhousesyncs and will surface any post-merge fallout on its own.
📄 Details: connector, baseline, pre-release, evidence plan, pre-flight, full run log
Connector and PR
| Connector | source-greenhouse (API source, manifest-only) |
| Version | 0.7.33 → 0.8.0 (MINOR, non-breaking) |
| PR | #83811 |
| Head tested | 3d9e70dd02e — the post-F01 commit, cursor filters restored on paginated requests |
| Pre-release | airbyte/source-greenhouse:0.8.0-preview.3d9e70d (publish run) |
| Comparison baseline | latest (auto) = 0.7.33, no override_control_image |
| Private detailed results | airbytehq/airbyte-internal-issues#16900 (comment) |
Why no known-bad baseline: this is proactive v3 migration prep, not a customer-reported regression, so no version is known-bad and today's published version is the only meaningful control. No major-version boundary is crossed.
What the change actually does (net vs. 0.7.33)
base_requester.url_base+ per-streampath→ one fully-qualifiedurlper stream. Same v1 endpoints.page_size_optionremoved;per_page: "{{ 100 if not next_page_token }}"added as a conditional request parameter, soper_pageis sent only on the first request of each partition.- Cursor
start_time_optionunchanged on this head — the incremental filter still goes out on every page. num_workersdescription reworded;api_budget/default_concurrencyuntouched.allowedHostsgainsauth.greenhouse.io; documentation reference URLs repointed.
Evidence plan
Hypothesis: output is byte-identical, so the comparison table should be all ✅ with no behavioral difference — the "no visible difference predicted" case, where proof has to come from the request shape.
Proving criteria (all met): page 2 carries no per_page on the PR version but does on the control; the cursor filter survives on both pages on both versions; pagination walks to completion; record counts, PKs, field values and state shape identical to control; and an affected paginated stream returns >100 records so the page-2 path is actually executed.
Disproving criteria (none observed): target emitting fewer records than control; a stream stopping after page 1; missing PKs, field-value diffs, Catalog schema ❌ or a state shape change; a target request URL differing from the control's beyond the intended per_page removal.
Strategy: regression tests only, no live pinning. source-greenhouse is a source authenticating with a Harvest API key (no forced-OAuth write-back), and the change is general request construction that any account with enough data exercises, so pinning a live customer connection would have added risk without adding signal. No connection was pinned and no customer sync was triggered — the harness reads a connection's config and catalog into an isolated CI run. Candidates were selected with exclude_pinned=True, non-EU only, internal Airbyte connections tried first.
Pre-flight checks
- Viability ✅ — the diff matches the linked issue's tasks; the change is declarative (no new Python component), and the
else-branch subtlety (per_page=Noneleaking as a literal parameter) was verified rather than assumed. - Safety ✅ — no obfuscation, no credential-handling change, no unexpected external calls. One new
allowedHostsentry,auth.greenhouse.io, which is Greenhouse's own documented v3 token host and is required before Phase 2. - Breaking change: none ✅ — checked against the full checklist on the actual diff, not the PR's claim: no field added/removed/renamed/retyped, no PK change, no cursor-field change, no spec field added or removed (description-only edit), no stream removed, no reduced data scope, no state-format change. Independently corroborated by
Spec compatibility✅ andCatalog schema✅ on every run that got that far. - Reversibility ✅ — manifest-and-metadata-only, no state or config format change, so
0.7.33reads anything0.8.0writes. Changelog present. Progressive rollout is not enabled on this connector and the PR does not change that. Correct MINOR bump. - Design intent ✅ — the one judgement call (whether to gate the cursor filter to first-page-only as well) was raised as F01 and resolved by the maintainer above; this head keeps the filter on every page, which is
0.7.33's existing behavior, and the capture above confirms that is what the code actually does.
Full attempt log — 5 attempts, 3 lost to unrelated infrastructure
| # | Setup | Outcome |
|---|---|---|
| 1 | Integration-test credentials, all 36 streams | ✅ every check green — but 403 Forbidden on both versions. Nothing paginated, so the change was never exercised. Green ≠ proven. |
| 2 | Internal Airbyte connection, all streams, cold read | ⚪ inconclusive — HTTP 401 Invalid Basic Auth credentials on both versions; that connection's Harvest key is expired. Salvage: debug logging showed target and control emitting byte-identical first-page URLs, confirming the per-stream url refactor reproduces the same host, path and query string. |
| 3 | Customer connection, all 36 streams, cold read | ⚪ READ hit the harness's 180-minute step timeout — a full-history cold read of 36 streams including per-application substreams does not fit the step budget on a real recruiting account. SPEC/CHECK/DISCOVER ✅. |
| 4 | Same, narrowed to applications, candidates |
⚪ READ cancelled externally at ~47 min, well short of the timeout, artifacts never uploaded. No concurrency block on the workflow and no newer run started before the cancellation, so this was not self-inflicted; the run-attempt metadata is not visible to this token, so runner eviction vs. manual cancel can't be distinguished. |
| 5 | Same, narrowed to jobs, offers, users |
🟢 fully green with real coverage — 2,192 records, all three streams past 100, target identical to control on every check. |
Narrowing streams was used only on the timeout-retry path. Attempts 2-4 are inconclusive rather than failures — a failure identical on the control, a timeout, and a cancellation provide no evidence for or against the PR, and the rules treat infrastructure losses as replaced attempts. Two incidental observations, neither a regression signal: the target logged a handful of 429 backoff-and-retry cycles the control did not, which is expected when two versions run sequentially against a 50-req/10s ceiling and the retries succeeded; and no run produced HAR captures, which is why the request-shape evidence was gathered deterministically instead.
Airbyte Support Bot (airbyte-support-bot)
added
the
hyd-prove
Co-Authored-By: bot_apk <apk@cognition.ai>
|
☑️ Resolved in 3d9e70d. F01's gate is dropped: each incremental stream's cursor is back on a typed The second-page assertion in This also settles F03 (typed cursor option instead of a Jinja literal). Description updated to match. Patrick Nilan (@pnilan) — for the |
|
There was a problem hiding this comment.
delete tests, unnecessary
There was a problem hiding this comment.
👍 On it. Removing test_pagination_request_params.py, and reverting the unit_tests/pyproject.toml + poetry.lock changes that existed only to pin the CDK for that test — so the connector goes back to having no unit-test suite of its own. Will update the PR's ## Test Coverage section to record that the request-shape evidence now comes from the /ai-prove-fix run (deterministic two-page capture + live comparison vs 0.7.33) rather than a shipped test.
There was a problem hiding this comment.
☑️ Resolved in 5e7cab9. Deleted test_pagination_request_params.py and reverted unit_tests/pyproject.toml + poetry.lock to master, so the connector diff is now just manifest.yaml + metadata.yaml (plus the docs file). PR description's Test Coverage and Review guide sections updated accordingly.
Co-Authored-By: bot_apk <apk@cognition.ai>
|
enable autopilot progressive rollouts for this pr |
…8.0-rc.1 Co-Authored-By: bot_apk <apk@cognition.ai>
|
☑️ Resolved in f2b7e49 — autopilot progressive rollout enabled.
releases:
rolloutConfiguration:
enableProgressiveRollout: true
defaultRolloutMode: autopilot
autopilotConfig:
autoStart: true
autoPromoteStages: true
strategy: fastSince progressive rollout requires the One thing to flag: the |
| connectorType: source | ||
| definitionId: 59f1e50a-331f-4f09-b3e8-2e8d4d355f44 | ||
| dockerImageTag: 0.8.0 | ||
| dockerImageTag: 0.8.0-rc.1 |
There was a problem hiding this comment.
don't need to -rc.X tag for autopilot progressive rollouts -- log that hydra feedback
There was a problem hiding this comment.
👍 On it. Reverting dockerImageTag and the changelog entry back to plain 0.8.0, keeping the rolloutConfiguration block. Then logging the Hydra feedback — the /ai-fix playbook's Step 4 currently states the -rc.x suffix is mandatory whenever enableProgressiveRollout: true and that pre-release checks fail without it, which is what I followed; that needs to be scoped to non-autopilot rollouts.
There was a problem hiding this comment.
☑️ Hydra feedback logged as well — missing_guidance against the issue_fix playbook, attributed to you, with auto-triage dispatched: https://github.com/airbytehq/airbyte-ops-mcp/actions/runs/31567133927
The specific text to fix is devin/playbooks/issue_fix.md lines 68-77 in airbytehq/ai-skills, which currently says the -rc.x suffix is required whenever enableProgressiveRollout: true and that pre-release checks fail without it — no carve-out for defaultRolloutMode: autopilot. That needs scoping to manual rollouts.
… need it Co-Authored-By: bot_apk <apk@cognition.ai>
|
Once this PR merges, close the linked github issue |
|
👍 On it (re: closing the linked issue on merge). Tracking merge on airbytehq/airbyte-internal-issues#16900 and will close it with a summary comment once this lands. The GitHub event-subscription backend is currently down ( |
|
☑️ Resolved. This merged, so airbytehq/airbyte-internal-issues#16900 is now closed as completed, with a wrap-up comment covering all five Phase 1 tasks, the two spec corrections (the literal- One item still needs a human: #83323, the earlier draft PR adding two Harvest v3 |
3 participants
What
Phase 1 of the Greenhouse Harvest v3 migration (Harvest v1/v2 is removed by Greenhouse on 2026-08-31). This lands everything that can be done while still calling Harvest v1, so the breaking Phase 2 PR shrinks to schema + auth only.
Resolves https://github.com/airbytehq/airbyte-internal-issues/issues/16900:
Parent epic: airbytehq/oncall#10401. Phase 2: airbytehq/airbyte-internal-issues#16901.
Not a breaking change (evaluated against the breaking-change checklist): no field added/removed/renamed/retyped, no PK or cursor-field change, no spec field added or removed, no stream removed, no state-format change, and no change to which records a stream returns. Endpoints, filter values and record output are unchanged. Released as MINOR,
0.7.33→0.8.0; nobreakingChangesentry. Autopilot progressive rollout is enabled on this connector as part of this PR (per review request); autopilot rollouts do not require an-rc.xversion suffix, so the version stays plain0.8.0.How
Per-stream fully-qualified
url— droppedbase_requester.url_baseand every streampath, replaced by oneurlper stream (url_base/pathare deprecated indeclarative_component_schema.yamlin favour ofurl). Same v1 endpoints, Jinja path segments preserved verbatim. Phase 2's endpoint repoint becomes one line per stream.Cursor-safe page size — Harvest v3 rejects any query parameter sent alongside
cursor(422), butpage_size_optionis injected on every request, including next-page requests (the paginator'spage_token_option: RequestPathalready supplies a complete next-page URL).page_size_optionwas removed and re-expressed as a conditional request parameter:Against v1 this is behaviourally identical: v1's next-page
LinkURL already carriesper_page(documented, and confirmed by a captured header), so the value on the wire is unchanged.per_pagestays at 100 here; Phase 2 raises it to v3's max of 500. The 6 streams that have no paginator today (activity_feed,approvals,disciplines,schools,tags,user_permissions) deliberately did not gain aper_page.The incremental cursor filter is deliberately not gated. An earlier revision of this PR also made
created_after/submitted_after/updated_afterfirst-page-only, as the issue asks. That was reverted on review: Greenhouse never documents which query parameters are carried into v1'srel="next"URL, and under v1'slimit/offsetpagination a page-2 request that lost its date filter would offset into the unfiltered collection and silently emit unrelated older records. The filter therefore stays on the typedstart_time_optionand is sent on every page, exactly as0.7.33does. Gating it belongs in Phase 2, where v3 bakes the filters into the opaque cursor and actually forbids them alongside it — see the documentation verification and Patrick Nilan (@pnilan)'s call. Net effect: Phase 1 pre-validates the page-size gate only.num_workersdescription reworded so it no longer hardcodes v1's "50 requests per 10 seconds".api_budgetanddefault_concurrencyare deliberately unchanged — the connector still calls v1, so the v1 ceiling still applies. The budget retune moves with the endpoint change in Phase 2.allowedHostsgainsauth.greenhouse.io(the v3 client-credentials token endpoint would otherwise be blocked on Cloud).Documentation URLs repointed to
harvestdocs.greenhouse.io:externalDocumentationUrlsinmetadata.yaml, and the per-stream reference links indocs/integrations/sources/greenhouse.md. Every new URL was checked for HTTP 200. Four streams have no v3 reference page and keep their existing v1 links: Activity Feed, Degrees, Disciplines, Schools. The two Harvest API-key setup links (greenhouse.mdlines 7 and 15) are intentionally left on v1 instructions — Phase 1 users still authenticate with a Harvest API key.Declarative-First Evaluation
Declarative only — no custom Python component was added or modified. The conditional
request_parametersinterpolation onHttpRequester, plus theurlfield and the existing typedRequestOption, deliver the whole change; no new behaviour needed Python.Decision on the overlapping PR
This supersedes #83323 (draft, adds two Harvest v3
externalDocumentationUrlsentries alongside the v1 ones). This PR repoints the same metadata block wholesale, so #83323 should be closed rather than merged; a comment saying so has been left there. #82701 (community draft adding a separatesource-greenhouse-harvest-v3connector) is untouched — this epic is an in-place migration, so the two are not merged together.Review guide
airbyte-integrations/connectors/source-greenhouse/manifest.yaml— the bulk; mechanical and repetitive across 36 streams.airbyte-integrations/connectors/source-greenhouse/metadata.yamldocs/integrations/sources/greenhouse.mdTest Coverage
No unit test ships with this PR. One was originally added (a two-page mock-HTTP test asserting the exact page-1 and page-2 requests for
applicationsand the parent-partitioned substreamapplications_interviews), along with a CDK pin bump inunit_tests/pyproject.toml/poetry.lockto match the base image. Both were removed at the maintainer's request — see the review thread onunit_tests/test_pagination_request_params.py— so the connector keeps having no unit-test suite of its own and the lockfile diff is gone. The evidence the test provided is covered by the prove-fix run below, which captured the same two-page request shapes deterministically against both this branch and0.7.33.Live regression run: done, and green. This session had no Greenhouse credentials, so the "no record diff vs the current release" acceptance criterion was initially left unverified. It has since been satisfied by a
/ai-prove-fixrun against this head (3d9e70dd02e, pre-release0.8.0-preview.3d9e70d) — see the prove-fix comment on this PR for the full evidence. Summary: comparison vs.0.7.33over 2,192 records on three streams that each page past 100 records (jobs830,offers779,users583) showed zero record delta, identical PKs and field values, unchanged catalog schema and state shape, and the only spec change being the intendednum_workers.descriptionreword. A separate deterministic two-page capture confirms page 2 carries the cursor filter and noper_pageon this branch while0.7.33still sendsper_pagethere.User Impact
None expected. Same endpoints, same records, same state; the only wire difference is that page-2+ requests no longer repeat
per_page, which v1's own next-page URL already carries. Users see only updated documentation links and a rewordednum_workersdescription.Can this PR be safely reverted and rolled back?
Link to Devin session: https://app.devin.ai/sessions/cb3111aa07fc44aea1658f96fab6c5cb
Important
⚡ Auto-merge enabled.
This PR is set to merge automatically when all requirements are met.