docs(trackers): publish compact adapter profiles - #772
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Coverage Report
File CoverageNo changed files found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28f45354c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Candidate polling scopes Project V2 items to configured active states using the | ||
| GitHub `query` argument. State and ID refreshes are unfiltered so terminal |
There was a problem hiding this comment.
Describe the terminal-exclusion query accurately
For workflows with additional non-active states such as Backlog, candidate polling is not scoped exclusively to active_states as stated here. githubProjectTrackerAdapter.listIssues() calls listProjectIssues() without a subsequent active-state filter, while buildTerminalStatesQuery() generates a negative query such as -status:Done; consequently, every non-terminal Project item can be returned. Either the adapter must filter to the requested active states or this profile must explicitly document the broader scope and resulting upstream divergence.
AGENTS.md reference: AGENTS.md:L36-L40
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Corrected the profile and Architecture §5: candidate polling excludes terminal states and can return other non-terminal items outside active_states.
| | `id` and `native_ref` | `id` is the Linear issue ID. `native_ref` contains that `itemId` and the configured `projectSlug`. | | ||
| | State, labels, priority, timestamps | State is the Linear workflow-state name. Labels are trimmed, lowercased, and deduplicated. Priority `0` (No priority) becomes `null`; other Linear values are retained. Timestamps parse to canonical RFC 3339/ISO 8601 or `null`. | | ||
| | `dispatchable` | Candidate polling includes unassigned issues. With `--assigned-only`, only the authenticated viewer's assigned issues are dispatchable; configured non-terminal blockers also make an item non-dispatchable with a reason. `pickup_labels` instead filters label-ineligible candidates from the list (a documented repository divergence from GitHub's retained records). | | ||
| | Malformed and optional fields | Candidate-state listing may omit malformed records with a diagnostic; requested ID refresh fails on malformed required data. Optional labels, assignee, timestamps, priority, and relations normalize to empty collections or `null`. | |
There was a problem hiding this comment.
Remove the unsupported malformed-record omission claim
When a candidate node has malformed required data, such as a missing state, listLinearIssues() maps every node through normalizeLinearIssue(), whose required-field checks throw and abort the entire polling call. There is no candidate-only catch or diagnostic that omits the bad record, so a single malformed item prevents all candidates from being returned rather than producing the partial-success behavior documented here.
Useful? React with 👍 / 👎.
| | `id` and `native_ref` | Fixture `id` is retained unchanged. `native_ref` is retained when supplied, otherwise it defaults to `{ itemId: tracker.itemId }`. | | ||
| | State, labels, priority, timestamps | Valid fixture values are preserved; fixture authors supply their normalized form. The adapter does not invent provider-specific state, label, priority, or timestamp mappings. | | ||
| | `dispatchable` | An omitted value defaults to `true`; `false` and `dispatchReason` can be supplied to exercise the adapter-neutral scheduler gate. Pickup-label filtering remains available through the shared normalized filter. | | ||
| | Malformed and optional fields | Entries lacking required shape (`id`, `identifier`, `state`, object `repository`, object `tracker`, or a boolean `dispatchable` when supplied) are skipped with a diagnostic. Optional `assigneeId` defaults to `null`. | |
There was a problem hiding this comment.
Disclose the file adapter's incomplete shape validation
A fixture missing a required normalized field such as title is accepted by isValidIssueShape() because it checks only the fields listed here, after which normalizeIssueDefaults() propagates the missing field as undefined. The resulting adapter output violates the upstream normalized-Issue contract, so this profile should not present the listed checks as the complete required shape without explicitly documenting that repository divergence or tightening validation.
AGENTS.md reference: AGENTS.md:L36-L40
Useful? React with 👍 / 👎.
hojinzs
left a comment
There was a problem hiding this comment.
Review 1/6 — Request changes
Docs-only change against issue #674. Structure and coverage are good; the blocker is accuracy in a few places where the profiles describe behavior the code does not have.
Requirements coverage vs #674
Every §11.2 item the issue enumerates is present in all three profiles: tracker.kind, provider keys + defaults + secretEnvironmentNames + validation errors, scope selection, pagination and request limits, id / native_ref mapping, normalization rules (state incl. synthetic Archived, label, priority, timestamp, dispatchable, malformed-record, optional-field), native tool summary, and the error-category table. Registration in docs/README.md and the docs/architecture.md §5 link are both done. No over-engineering or scope creep — the diff is 5 files and stays documentation.
Verification performed
I checked the factual claims against the adapters rather than reading for prose. Confirmed correct:
| Claim | Source |
|---|---|
| GitHub page size 25, 30s timeout | adapter.ts:30-31 |
| Linear 50 / 100 pages / cap 1,000 / 10s / cap 60s | orchestrator-adapter.ts:21-25 |
secretEnvironmentNames for all three adapters |
tracker-github:44, tracker-linear:236, tracker-file:163 |
Linear rejects project_id/projectId/teamId/team_id |
orchestrator-adapter.ts:212-222 |
Linear api_key must be $NAME / env:NAME / ${NAME} |
orchestrator-adapter.ts:423 |
Linear endpoint default, priority 0 → null |
parser.ts:315, orchestrator-adapter.ts:660-662 |
| GitHub provider key list, endpoint HTTP(S), object/list key validation | orchestrator-adapter.ts:354-433 |
GitHub synthetic Archived |
adapter.ts:395 |
File defaults Status / Ready,In Progress / Done,Cancelled |
file-tracker-adapter.ts:153-161 |
| File malformed-shape skip + optional-field defaults | file-tracker-adapter.ts:30-58 |
Smoke test
Docs-only, so the meaningful checks are link and format integrity:
- All relative links in the 5 changed files resolve to existing targets — pass
npx prettier --checkon all 5 changed files — pass- CI on
28f4535: Test ✅, Container Smoke ✅
Findings
| # | Sev | Item |
|---|---|---|
| 1 | P1 | Error-category tables document a taxonomy that isn't implemented — 6 of 7 categories are emitted nowhere in the codebase |
| 2 | P2 | GitHub rate-limit "capped at 60 seconds" describes a clamp-then-retry; the code actually abandons the retry past 60s |
| 3 | P3 | tracker.provider.path documented as file scope selector but never read |
| 4 | P3 | file.md merges a parse-time WorkflowValidationError and a run-time plain Error into one row |
| 5 | P3 | trackers/github.md dropped from the docs index while still live and linked |
| 6 | P3 | Package READMEs retain competing "Adapter profile" sections after §5 was repointed |
| 7 | nit | Blocker-check default derives from activeStates[0], not defaultLifecycle() |
| 8 | nit | native_ref fallback is { itemId: null } for a non-string tracker.itemId |
Details inline.
Why this blocks
Only #1 is a real blocker. These profiles are the §11.2 compliance artifact and github-project.md:5 asserts they describe "current Integration-layer behavior" — but of the seven §11.4 categories, only tracker_pagination is emitted by any adapter. The rest exist solely in symphony-spec.md and the drift report. Publishing them as the adapters' error contract converts a known gap into apparent coverage, which is the drift this epic exists to remove. The PR's own unchecked box — "Confirm no provider behavior has been inferred beyond the adapter implementation" — is the exact box this fails.
The fix is small and stays documentation: keep the tables (the issue requires them) and mark which rows are target vs. implemented. #2 is a similarly cheap rewording but worth getting right, since it is the paragraph an operator reads during a rate-limit incident.
P3s and nits are non-blocking — take or leave them with a note.
Generated by Claude Code
| | Provider-native failure | Adapter category | | ||
| | ------------------------------------------------------------------- | ------------------------ | | ||
| | Network/transport failure or timeout | `tracker_request` | | ||
| | Non-rate-limited HTTP status | `tracker_status` | | ||
| | Invalid JSON, GraphQL errors, or missing expected response data | `tracker_response` | | ||
| | Missing cursor or incomplete cursor traversal | `tracker_pagination` | | ||
| | GitHub primary/secondary quota exhaustion or `Retry-After` response | `tracker_rate_limited` | |
There was a problem hiding this comment.
P1 — this table documents an error taxonomy the adapter does not emit.
I grepped every §11.4 category across packages/ and docs/. Only tracker_pagination is actually produced by code:
packages/tracker-github/src/adapter.ts:2442—Object.assign(error, { category: "tracker_pagination", event })packages/tracker-linear/src/orchestrator-adapter.ts:702— same
The other six (tracker_request, tracker_status, tracker_response, tracker_rate_limited, invalid_tracker_config, missing_tracker_secret) appear only in docs/symphony-spec.md and the drift report. Nothing in any adapter emits them. What the GitHub adapter actually throws is a class hierarchy with no category field at all:
// packages/tracker-github/src/adapter.ts:370
export class GitHubTrackerHttpError extends GitHubTrackerError {
constructor(message, readonly status, readonly details, rateLimits = null, readonly retryAfterMs = null)
}
export class GitHubTrackerQueryError extends GitHubTrackerError {}And config validation raises WorkflowValidationError with the code "workflow_validation_error" (orchestrator-adapter.ts:379), not invalid_tracker_config.
This matters because line 5 of this file states the profile "describes the repository's current Integration-layer behavior." A reader writing switch (err.category) against this table gets exactly one working case out of seven. It also leaves the PR's own unchecked box — "Confirm no provider behavior has been inferred beyond the adapter implementation" — failing.
Issue #674 does require the mapping table, so I'm not asking to remove it. Please distinguish target from current, e.g. a header note along the lines of:
Categories below are the §11.4 target mapping. Today only
tracker_paginationis emitted as a structuredcategory; the remaining rows describe failures currently surfaced asGitHubTrackerHttpError/GitHubTrackerQueryError/WorkflowValidationErrorand are not yet normalized.
Same applies to the equivalent tables in linear.md and file.md.
Generated by Claude Code
There was a problem hiding this comment.
Updated all three tables to label the §11.4 mappings as targets and state the current unnormalized error surfaces; only tracker_pagination is emitted structurally today.
| `timeoutMs`). It records GraphQL rate-limit metadata and applies the shared | ||
| GitHub policy: honor `Retry-After`, otherwise wait only until the known primary | ||
| reset, capped at 60 seconds, before retrying rate-limited requests. |
There was a problem hiding this comment.
P2 — the rate-limit policy description inverts what the code does on a long reset.
Two inaccuracies in "honor Retry-After, otherwise wait only until the known primary reset, capped at 60 seconds, before retrying":
-
Not "otherwise" — it takes the later of the two.
packages/tool-github-graphql/src/github-rate-limit.ts:173:const retryAtMs = maxNullable(primaryResetAtMs, retryAfterAtMs);
And
primaryResetAtMsis only non-null whenremaining <= hardThreshold(line 168–172), so the primary reset is not always "known" in the sense implied here. -
"capped at 60 seconds, before retrying" is the important one. The 60s bound is not a clamp on the wait — it is a cutoff that cancels the retry:
// github-rate-limit.ts:192-195 const delayMs = Math.max(exponentialMs, providerWaitMs); return delayMs <= this.maxWaitMs() ? delayMs : null; // null ⇒ no retry
GitHub primary rate-limit resets are routinely far more than 60 seconds out. As written, an operator reads "we wait at most a minute, then retry"; what actually happens is the adapter gives up and surfaces the rate-limit error. That is the opposite operational expectation during the exact incident this paragraph exists to explain.
Suggested rewording: "honors the later of Retry-After and the known primary reset; if the required wait exceeds 60 seconds the request is not retried and the rate-limit error is surfaced."
Generated by Claude Code
There was a problem hiding this comment.
Corrected the rate-limit text: it uses the later of Retry-After and the known primary reset, and surfaces the error without retrying when the required delay exceeds 60 seconds.
| | -------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | `tracker.kind` | `file` | | ||
| | Provider scope | A local JSON issue array selected by the adapter's configured issues path; it has no project, board, team, or remote repository selection. | | ||
| | Provider keys | `tracker.provider.path` is validated when present; the normalized runtime setting supplies the required issues path. Unknown provider keys are preserved by core parsing. | |
There was a problem hiding this comment.
P3 — tracker.provider.path is validated but never read; the phrasing invites people to configure a dead key.
The adapter validates provider.path (file-tracker-adapter.ts:139-151) but every read path uses a different key from a different config section:
// file-tracker-adapter.ts:88, 219, 279
const issuesPath = requireTrackerSetting(project, "issuesPath"); // project.tracker.settings.issuesPathgrep -rn "provider\.path" in packages/tracker-file/src returns only the two validator lines. The value is supplied entirely by the CLI (cli/src/repo-runtime.ts:145, cli/src/commands/project.ts:371 via resolveFileTrackerIssuesPath).
So setting tracker.provider.path in a workflow passes validation and then silently does nothing — a fixture author would reasonably expect it to select the file. "the normalized runtime setting supplies the required issues path" is technically true but doesn't warn anyone off. Please say plainly that path is accepted for forward compatibility and currently inert, and name tracker.settings.issuesPath as the effective key.
Generated by Claude Code
There was a problem hiding this comment.
Documented tracker.provider.path as validated but currently inert, and named tracker.settings.issuesPath as the effective setting.
| | No corresponding provider HTTP status | `tracker_status` (not applicable) | | ||
| | No pagination | `tracker_pagination` (not applicable) | | ||
| | No remote quota | `tracker_rate_limited` (not applicable) | | ||
| | Invalid `path` or missing required issues-path setting | `invalid_tracker_config` | |
There was a problem hiding this comment.
P3 — this row merges two failure modes that behave differently.
- Invalid
path→WorkflowValidationError("workflow_validation_error", "tracker.provider.path", …), raised at config-parse time (file-tracker-adapter.ts:143). - Missing issues-path → a plain
Errorthrown at operation time, once per read/write call:// file-tracker-adapter.ts:19-21 throw new Error(`Tracker adapter "file" requires the "${key}" setting.`);
Different error type, different lifecycle stage, and neither carries invalid_tracker_config. Worth splitting into two rows so the E2E behavior is legible — a fixture with no issuesPath fails mid-run, not at load.
Generated by Claude Code
There was a problem hiding this comment.
Split the parse-time invalid provider.path WorkflowValidationError from the per-operation missing issuesPath plain Error.
| - [GitHub Project](trackers/github-project.md) — configuration, normalization, and `github_graphql` | ||
| - [Linear](trackers/linear.md) — configuration, normalization, and `linear_graphql` | ||
| - [File](trackers/file.md) — local/Docker E2E fixture adapter profile |
There was a problem hiding this comment.
P3 — trackers/github.md just fell out of the living-documents index but is still live.
The old - [GitHub](trackers/github.md) — github_graphql entry was replaced rather than joined by the new one. The file still exists and is still referenced from two places:
docs/trackers/github-project.md:45— "The host-sidegithub_graphqltool…"docs/adr/2026-08-28_agent-tool-isolation.md:217
Issue #674's acceptance criteria is registration in the docs/README.md living documents list, so leaving a still-referenced tracker doc unlisted works against the point of the change. Note the asymmetry too: linear.md keeps its tool contract inline and stays indexed, while GitHub's tool contract now lives in an unindexed sibling. Either add github.md back as its own entry, or fold it into github-project.md the way linear.md does.
Generated by Claude Code
There was a problem hiding this comment.
Restored docs/trackers/github.md as a separate GitHub tool entry in the living-documents index.
| - Compact adapter profiles: [GitHub Project](trackers/github-project.md), | ||
| [Linear](trackers/linear.md), and [file](trackers/file.md). GitHub's | ||
| synthetic `Archived` state is a GitHub-specific implementation choice, not | ||
| normalized Symphony core behavior. |
There was a problem hiding this comment.
P3 — repointing §5 away from the package README leaves two competing "adapter profile" sources.
This hunk moves the canonical pointer from packages/tracker-github/README.md to the new docs, but both package READMEs still carry their own profile prose:
packages/tracker-github/README.md— an## Adapter profilesection plus an "Adapter profile note:" line covering label/timestamp normalization andtracker_paginationpackages/tracker-linear/README.md— an "Adapter profile note:" line covering labels, timestamps, and priority0→null
They agree with the new profiles today, so nothing is wrong right now — but two independently-editable copies of the same normalization rules is exactly how the drift this epic is closing got started. Suggest reducing the package READMEs to a one-line pointer at the canonical docs/trackers/*.md profile.
Generated by Claude Code
There was a problem hiding this comment.
Reduced both package READMEs to pointers to their canonical compact profiles, removing duplicate normalization and pagination prose.
| | `tracker.kind` | `github-project` | | ||
| | Provider scope | `tracker.provider.project_id` selects one GitHub Project V2. `repository` is derived from each issue; the optional runtime repository filter and `--assigned-only` are adapter dispatchability rules. | | ||
| | Provider keys | `project_id`, `endpoint`, `state_field`, `priority_field`, `priority`, `pickup_labels`, `active_states`, `terminal_states`, `blocker_check_states`, and `planning_states`. Unknown provider keys are preserved by core configuration parsing. Flat `tracker.*` keys are deprecated compatibility aliases. | | ||
| | Defaults | Lifecycle defaults are `Status`, active `Todo`/`In Progress`, terminal `Done`, blocker checks in `Todo`, and no planning states. Priority is `null` unless the configured `priority` policy or deprecated `priority_field` resolves a value. | |
There was a problem hiding this comment.
nit — "blocker checks in Todo" is true by coincidence, not because it is an adapter default.
All three adapters do declare blockerCheckStates in defaultLifecycle(), but the parser ignores that field entirely:
// packages/core/src/workflow/parser.ts:211-217
const blockerCheckStates =
readNormalizedStringList(tracker, provider, explicitProviderKeys, "blocker_check_states")
?? (activeStates[0] ? [activeStates[0]] : []);Note the other lifecycle fields fall through to defaultLifecycle?.…; this one falls through to activeStates[0]. It lands on Todo here (and Ready in file.md) only because that happens to be the first default active state.
The consequence for a reader: set active_states: [Backlog, In Progress] without setting blocker_check_states, and blocker checks run in Backlog — not the Todo this row advertises as the default. Worth phrasing as "blocker checks default to the first active state" in all three profiles.
(The dead blockerCheckStates in defaultLifecycle() looks like a genuine code smell, but it predates this PR — flagging only so the docs don't cement it.)
Generated by Claude Code
There was a problem hiding this comment.
Corrected all profile defaults to state that blocker checks use the first active state unless configured explicitly.
|
|
||
| | Field or condition | File mapping | | ||
| | ----------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | `id` and `native_ref` | Fixture `id` is retained unchanged. `native_ref` is retained when supplied, otherwise it defaults to `{ itemId: tracker.itemId }`. | |
There was a problem hiding this comment.
nit — the native_ref fallback is not quite { itemId: tracker.itemId }.
// file-tracker-adapter.ts:50-53
const nativeRef =
entry.nativeRef && typeof entry.nativeRef === "object"
? entry.nativeRef
: { itemId: typeof tracker.itemId === "string" ? tracker.itemId : null };A non-string tracker.itemId yields { itemId: null } rather than propagating the raw value. Minor, but this is the row a fixture author reads when a native_ref comes back empty.
Generated by Claude Code
There was a problem hiding this comment.
Corrected the file native_ref fallback: it retains a string tracker.itemId, otherwise uses itemId: null.
hojinzs
left a comment
There was a problem hiding this comment.
Review 2/6 — Request changes
All eight findings from Review 1/6 are genuinely fixed, and I verified each against the code rather than taking the reply at face value. What blocks now is one unaddressed Codex finding that turns out to be correct, plus one inaccuracy introduced by the P1 fix itself.
Round 1 findings — verified fixed
| # | Sev | Fix verified against |
|---|---|---|
| 1 | P1 | All three tables now carry a target-vs-current header note and a "Current adapter surface" column. Matches reality: only tracker_pagination is a structured category (tracker-github/src/adapter.ts:2439, tracker-linear/src/orchestrator-adapter.ts:702) |
| 2 | P2 | "honors the later of Retry-After and a known primary reset… if the required wait exceeds 60 seconds, the request is not retried" — matches github-rate-limit.ts:173 (maxNullable) and :192-195 (delayMs <= maxWaitMs ? delayMs : null) |
| 3 | P3 | provider.path now stated as "accepted and validated for forward compatibility but currently inert", with tracker.settings.issuesPath named as effective |
| 4 | P3 | Split into parse-time WorkflowValidationError and per-operation plain Error rows |
| 5 | P3 | docs/trackers/github.md restored to the index as its own "GitHub tool" entry |
| 6 | P3 | Both package READMEs reduced to canonical pointers; duplicate normalization prose removed |
| 7 | nit | All three profiles now say blocker checks use the first active state unless configured (core/src/workflow/parser.ts:211-217) |
| 8 | nit | native_ref fallback corrected to string-itemId-or-null (file-tracker-adapter.ts:50-53) |
The Codex github-project.md terminal-exclusion thread is also correctly fixed, in both the profile and Architecture §5.
Smoke test
Docs-only, so link and format integrity plus CI:
- Relative
.mdlinks across all 7 changed files resolve to existing targets — pass npx prettier --checkon all 7 changed files — pass- CI on
78f56ef: Test ✅, Container Smoke ✅
Findings
| # | Sev | Item |
|---|---|---|
| 1 | P1 | linear.md documents a candidate-skip / refresh-fail split that doesn't exist — malformed data aborts both paths |
| 2 | P2 | github-project.md rate-limit row names GitHubTrackerHttpError; the actual surface is GitHubGraphQLRateLimitError |
| 3 | P3 | github-project.md conflates the silent unsupported-content skip with the event-emitting missing-state skip |
| 4 | P3 | file.md "required shape" reads as exhaustive; title is unchecked (Codex thread, unanswered) |
Details inline.
Why this blocks
Two of Codex's three findings were fixed on this push; the linear.md one (#1 here) and the file.md one (#4) were left open with no change and no reply. #1 is the blocker on its own merits — listLinearIssues() runs every node through normalizeLinearIssue() with no catch, so a single malformed Linear issue stalls the whole poll, not just that record. The profile promises the opposite.
#2 is worth fixing carefully because it cuts the other way from round 1: there I flagged the tables for overstating normalization, and this row now understates it — GitHubGraphQLRateLimitError extends TrackerRateLimitError, a core contract type. Getting it right is the same accuracy standard, not a new one.
Both are small text edits. #3 and #4 are non-blocking — take them or reply with a reason.
Process note: please reply on the two open Codex threads rather than leaving them silent. You answered all eight of mine, which made this round much faster to verify; the same courtesy for the bot threads keeps the audit trail complete.
Generated by Claude Code
| | `id` and `native_ref` | `id` is the Linear issue ID. `native_ref` contains that `itemId` and the configured `projectSlug`. | | ||
| | State, labels, priority, timestamps | State is the Linear workflow-state name. Labels are trimmed, lowercased, and deduplicated. Priority `0` (No priority) becomes `null`; other Linear values are retained. Timestamps parse to canonical RFC 3339/ISO 8601 or `null`. | | ||
| | `dispatchable` | Candidate polling includes unassigned issues. With `--assigned-only`, only the authenticated viewer's assigned issues are dispatchable; configured non-terminal blockers also make an item non-dispatchable with a reason. `pickup_labels` instead filters label-ineligible candidates from the list (a documented repository divergence from GitHub's retained records). | | ||
| | Malformed and optional fields | Candidate-state listing may omit malformed records with a diagnostic; requested ID refresh fails on malformed required data. Optional labels, assignee, timestamps, priority, and relations normalize to empty collections or `null`. | |
There was a problem hiding this comment.
P1 — the candidate-vs-refresh contrast in this row does not exist in the code. (Codex raised this on 28f4535; the thread is still open with no reply and no change.)
Both paths are the same function. listIssues() (orchestrator-adapter.ts:286), the state-filtered candidate listing (:311), and the ID refresh (:319) all call listLinearIssues(), which has exactly one normalization step:
// orchestrator-adapter.ts:505-510
const fetchedIssues = result.nodes.map((node) =>
normalizeLinearIssue(project, config.projectSlug, node, { ... })
) as TrackedIssueList;No try/catch, no filter, no diagnostic. And normalizeLinearIssue throws on any missing required field:
// orchestrator-adapter.ts:648-651
const id = requireString(issue.id, "Linear issue id");
const identifier = sanitizeLinearIdentifier(requireString(issue.identifier, "Linear issue identifier"));
const state = requireString(issue.state?.name, "Linear issue state name");Page collection doesn't filter either — issues.push(...(connection?.nodes ?? [])) (:578).
So one malformed node aborts the entire poll, candidate listing included. The row promises partial success on the candidate path and failure only on refresh; the real behavior is failure on both. That is the opposite operational expectation: an operator reading this would assume a single bad Linear issue degrades the candidate set, when it actually stalls dispatch for the whole project.
Note the asymmetry this row implies is real elsewhere — tracker-file genuinely does skip-with-diagnostic (file-tracker-adapter.ts:93-97) and tracker-github genuinely does emit a structured event for missing-state items. Linear is the one adapter that does neither, which is probably how this got written.
Suggested: "Malformed required data (id, identifier, or state name) aborts the whole listing, on both candidate polling and ID refresh; there is no per-record skip. Optional labels, assignee, timestamps, priority, and relations normalize to empty collections or null."
Generated by Claude Code
There was a problem hiding this comment.
Corrected in 70e8590. The profile now says malformed required Linear data aborts the whole listing for both candidate polling and ID refresh, with no per-record skip.
| | Non-rate-limited HTTP status | `tracker_status` | `GitHubTrackerHttpError` | | ||
| | Invalid JSON, GraphQL errors, or missing expected data | `tracker_response` | `GitHubTrackerQueryError` | | ||
| | Missing cursor or incomplete cursor traversal | `tracker_pagination` | Structured `tracker_pagination` | | ||
| | GitHub quota exhaustion or `Retry-After` response | `tracker_rate_limited` | `GitHubTrackerHttpError` | |
There was a problem hiding this comment.
P2 — this row names the wrong error class, and in doing so understates what the adapter already normalizes.
A rate-limited response never reaches the GitHubTrackerHttpError conversion. githubGraphQLRateLimitPolicy.execute() throws on the rate-limited branch and only returns ok: false when the failure is not rate-limited:
// tool-github-graphql/src/github-rate-limit.ts:121-135
if (result.ok || !result.rateLimited) {
return result; // ← the only path that reaches GitHubTrackerHttpError
}
const error = this.buildRateLimitError(result);
if (retry + 1 >= retryAttempts) throw error;
const delayMs = this.resolveRetryDelay(error, retry);
if (delayMs === null) throw error;The GitHubTrackerHttpError at adapter.ts:2893 is guarded by if (!requestResult.ok), so it can only ever see non-rate-limited statuses — which is exactly what the row above it (tracker_status) already covers.
What actually propagates is GitHubGraphQLRateLimitError, and it is not an unnormalized native error:
// github-rate-limit.ts:74
export class GitHubGraphQLRateLimitError extends TrackerRateLimitErrorTrackerRateLimitError is a core contract type (core/src/contracts/tracker-adapter.ts:10) carrying rateLimits and retryAt. It has no category field, so the section header's "only tracker_pagination is emitted as a structured category" stays correct — but "Current adapter surface: GitHubTrackerHttpError" is wrong twice over: wrong class, and it hides that this row is already partly normalized through core.
The practical cost: this is the row someone consults while writing rate-limit handling, and a catch (e) { if (e instanceof GitHubTrackerHttpError) … } written from it misses every rate-limit error.
Suggested: `GitHubGraphQLRateLimitError` (core `TrackerRateLimitError`, no `category`).
Generated by Claude Code
There was a problem hiding this comment.
Corrected in 70e8590. The rate-limit row now names GitHubGraphQLRateLimitError and identifies its core TrackerRateLimitError base type while retaining the distinction that it has no structured category.
| | Labels and priority | Labels are trimmed, lowercased, deduplicated, and sorted. Priority is explicit Project-field or label policy output; unmapped/disabled values are `null`. | | ||
| | Timestamps | Issue and Project-item timestamps are parsed to canonical RFC 3339/ISO 8601; invalid or absent values become `null`. The newer item timestamp wins. | | ||
| | `dispatchable` | Starts `true`, then the adapter applies assignment, repository, fork-PR, pickup-label, and configured blocker rules. Ineligible items remain listed with `dispatchable: false` and an explainable `dispatchReason`. | | ||
| | Malformed and optional fields | Candidate polling skips unsupported/malformed Project items and emits a structured event; requested ID refreshes fail instead. Optional body, labels, assignee, timestamps, priority, and linked metadata normalize to `null`, empty collections, or documented defaults rather than leaking provider shape. | |
There was a problem hiding this comment.
P3 — "skips unsupported/malformed items and emits a structured event" conflates two skips, only one of which is observable.
normalizeProjectItem has two distinct return null paths:
// adapter.ts:406-411 — unsupported content type: silent
if (item.content?.__typename !== "Issue" && item.content?.__typename !== "PullRequest") {
return null;
}
// adapter.ts:421-428 — missing state: emits an event
if (getMissingStateSkippedItem(item, lifecycle)) {
emitProjectItemStatusMissingEvent({ projectId, itemId: item.id, issueIdentifier });
return null;
}Only the second emits an event, and only the second lands in skippedItems (:635-641, surfaced to the orchestrator at service.ts:1228-1232 as a skip count and log line). A draft Project item, or a ProjectV2Item with no content, vanishes with no event, no skippedItems entry, and no contribution to the skip count — so an operator reconciling "the Project has 40 items, the poll returned 37" gets no signal for that class at all.
Worth splitting: missing configured state → skipped, structured event, counted in skippedItems; non-Issue/PR content → dropped silently.
Generated by Claude Code
There was a problem hiding this comment.
Corrected in 70e8590. The profile now separates silently dropped non-Issue/PR content from missing configured state, which emits the structured event and is recorded in skippedItems; it also retains the ID-refresh failure behavior.
| | `id` and `native_ref` | Fixture `id` is retained unchanged. `native_ref` is retained when supplied; otherwise it defaults to `{ itemId: tracker.itemId }` only for a string item ID, or `{ itemId: null }`. | | ||
| | State, labels, priority, timestamps | Valid fixture values are preserved; fixture authors supply their normalized form. The adapter does not invent provider-specific state, label, priority, or timestamp mappings. | | ||
| | `dispatchable` | An omitted value defaults to `true`; `false` and `dispatchReason` can be supplied to exercise the adapter-neutral scheduler gate. Pickup-label filtering remains available through the shared normalized filter. | | ||
| | Malformed and optional fields | Entries lacking required shape (`id`, `identifier`, `state`, object `repository`, object `tracker`, or a boolean `dispatchable` when supplied) are skipped with a diagnostic. Optional `assigneeId` defaults to `null`. | |
There was a problem hiding this comment.
P3 — the enumerated list is accurate, but "required shape" reads as complete when it isn't. (Codex's open thread; still no reply.)
The list matches isValidIssueShape exactly (file-tracker-adapter.ts:30-42), and the skip-with-diagnostic claim checks out (:93-97, process.stderr.write("[tracker-file] Skipping invalid issue at index …")). So nothing here is false.
The gap is what the phrase implies. title is required on TrackedIssue but unchecked, and normalizeIssueDefaults spreads and casts:
// file-tracker-adapter.ts:44-58
return { ...entry, nativeRef, dispatchable: …, assigneeId: … } as TrackedIssue;A fixture without title therefore passes validation and reaches the orchestrator with title: undefined — a normalized-contract violation that the as TrackedIssue cast hides from the compiler.
I'm not asking you to tighten the validator in a docs PR; that's a code change and out of scope here. But since the profile is the §11.2 compliance artifact, either add a half-sentence ("this shape check is not exhaustive — e.g. a missing title is not rejected") or reply here saying it's a known code gap and file it. Please also reply on Codex's thread either way — leaving it silent is what the review-response convention is meant to prevent.
Generated by Claude Code
There was a problem hiding this comment.
Corrected in 70e8590. The profile now calls this the checked—not exhaustive—shape and explicitly notes that a missing title is currently not rejected. This documents the existing implementation gap without expanding this documentation-only change into a validator change.
hojinzs
left a comment
There was a problem hiding this comment.
Review 3/6 — Approve
All four Review 2/6 findings are fixed, and I verified each against the adapters rather than accepting the reply. Nothing new blocks; one nit inline that you can take or leave.
Round 2 findings — verified fixed
| # | Sev | Fix verified against |
|---|---|---|
| 1 | P1 | linear.md now: "Malformed required data (id, identifier, or state name) aborts the whole listing in both candidate polling and ID refresh; there is no per-record skip." Matches orchestrator-adapter.ts:505-511 — result.nodes.map(normalizeLinearIssue…) with no try/catch and no filter |
| 2 | P2 | Rate-limit row now reads `GitHubGraphQLRateLimitError` (core `TrackerRateLimitError`, no `category`). Confirmed at github-rate-limit.ts:74 — extends TrackerRateLimitError. The row keeps the no-category distinction, so it neither overstates nor understates |
| 3 | P3 | Split correctly: non-Issue/PR content dropped silently (adapter.ts:406-411), missing configured state skipped with structured event and recorded in skippedItems (:422-423, :635-640, :700) |
| 4 | P3 | file.md now says "the checked shape" and states plainly that a missing title is currently not rejected |
The two previously-silent Codex threads (linear.md, file.md) both have replies now. Audit trail is complete.
Smoke test
Docs-only, so link and format integrity plus CI:
- All relative
.mdlinks across the 7 changed files resolve to existing targets — pass npx prettier --checkon all 7 changed files — passgit diff --check— clean- CI on
70e8590: Test ✅, Container Smoke ✅
Requirements coverage vs #674
Re-confirmed unchanged from Review 1/6: all §11.2 items present in all three profiles, docs/README.md registration and the docs/architecture.md §5 link both in place. The last two pushes only corrected accuracy — nothing required by the issue was removed. Still no scope creep: 7 files, documentation only, no runtime or config-parsing change.
Why this approves
The accuracy standard I set in rounds 1 and 2 is met. Notably the profiles now disclose three real implementation gaps rather than papering over them — the unimplemented §11.4 categories, Linear's whole-poll abort on a malformed record, and the file adapter's non-exhaustive shape check. That is the right outcome for a §11.2 compliance artifact under an epic whose purpose is removing drift: it documents what the adapters do, and makes the gaps legible for follow-up issues instead of hiding them behind normalized-sounding prose.
The one open item is a nit — the error-table intro enumerates three current surfaces and the table now has four. Not worth another round.
Two things worth filing separately, both pre-existing and correctly kept out of this docs PR: the dead blockerCheckStates in defaultLifecycle() (round 1, nit 7) and the file-tracker-adapter as TrackedIssue cast that lets a title-less fixture through (round 2, #4).
Generated by Claude Code
| The other rows describe the intended normalized category; current failures are | ||
| surfaced as `GitHubTrackerHttpError`, `GitHubTrackerQueryError`, or |
There was a problem hiding this comment.
nit — this intro now under-counts the table below it by one. Non-blocking; noting it only because the round-2 fix created it.
The sentence enumerates three current surfaces as if exhaustive, but the tracker_rate_limited row you corrected in 70e8590 names a fourth — GitHubGraphQLRateLimitError — and it is the one that is partly normalized (it extends core TrackerRateLimitError), which is exactly the distinction the corrected row exists to make. A reader who stops at the intro concludes nothing here touches core contracts.
Suggested: …surfaced as GitHubTrackerHttpError, GitHubTrackerQueryError, GitHubGraphQLRateLimitError, or WorkflowValidationErrorand are not yet normalized to acategory(thoughGitHubGraphQLRateLimitErrordoes extend the coreTrackerRateLimitError).
Take it or leave it — approving either way.
Generated by Claude Code
70e8590 to
7c62dc1
Compare
Issues — Closed #674
TL;DR
Publishes compact Symphony §11.2 profiles for the GitHub Project, Linear, and file tracker adapters, aligned to their actual current configuration, error, malformed-record, and normalization behavior.
Change-point diagram
tracker implementation→canonical adapter profile→docs index + Architecture §5 + package README pointersStart here
docs/trackers/github-project.md: GitHub Project configuration, normalization, exact rate-limit error surface, and distinct malformed-item outcomes.docs/trackers/linear.md: Linear configuration, normalization, pagination, and whole-poll malformed-record behavior.docs/trackers/file.md: local/Docker E2E fixture semantics and the explicitly non-exhaustive shape check.Changed files
GitHubGraphQLRateLimitError; GitHub skip paths differ; and file fixture shape validation is non-exhaustive.Evidence
pnpm exec prettier --check docs/trackers/github-project.md docs/trackers/linear.md docs/trackers/file.md docs/README.md docs/architecture.md packages/tracker-github/README.md packages/tracker-linear/README.md— passgit diff --check— passpnpm lint— passpnpm test— passpnpm typecheck— passpnpm build— passRisks and rollback
Documentation-only change. Profiles intentionally disclose existing implementation gaps rather than representing them as normalized adapter behavior. Roll back by reverting
70e8590and the prior profile commits; no runtime behavior or configuration parsing changes.Post-merge / human validation
Human validation