Harden factory SDK cloud writeback payloads#243
Conversation
|
Warning Review limit reached
More reviews will be available in 52 minutes and 45 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds guarded write semantics to mount writes, enforces draft predicates for provider writeback, tightens provider acknowledgement semantics, updates writeback adapters to use guarded writes plus ack+readback verification, installs factory-scope draft authorization in CLI, and requires reconciled real Linear issues before orchestrator dispatch. Tests added/updated across these areas. ChangesGuarded Writeback Mechanism
Factory-Scope Draft Authorization and Real-Issue Validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces draft validation and safety guards for provider writebacks. It adds an isAllowedDraft predicate to RelayfileCloudMountClient to prevent unauthorized drafts, updates FactoryLoop to skip unreconciled Linear issues, and refactors comment paths to be stored under /linear/comments/. The reviewer feedback highlights three key issues: returning 'timeout' instead of 'acked' when an operation ID is missing could break writebacks; checking for issue paths using only startsWith may fail to match issues without UUID suffixes; and a check for /comments/ inside /linear/issues/ paths is now obsolete and should be removed.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const opId = this.#lastOpByPath.get(path) | ||
| if (!opId || !this.#client.getOp) return 'acked' | ||
| if (!opId || !this.#client.getOp) return 'timeout' |
There was a problem hiding this comment.
If opId is not present in this.#lastOpByPath (e.g., because the client was restarted or it is a different instance), returning 'timeout' will cause all subsequent verifications or confirmations to fail with a timeout. Furthermore, if this.#client.getOp is not implemented (since it is optional in RelayFileClientLike), returning 'timeout' will break all writebacks. Returning 'acked' as a fallback is safer and matches the previous behavior.
| const opId = this.#lastOpByPath.get(path) | |
| if (!opId || !this.#client.getOp) return 'acked' | |
| if (!opId || !this.#client.getOp) return 'timeout' | |
| const opId = this.#lastOpByPath.get(path) | |
| if (!opId || !this.#client.getOp) return 'acked' |
| const candidates = await mount.listTree('/linear/issues/') | ||
| const issuePath = candidates.find((candidate) => candidate.startsWith(`/linear/issues/${issueKey}__`)) | ||
| if (!issuePath) return false |
There was a problem hiding this comment.
If the issue file is named /linear/issues/${issueKey}.json (without the UUID suffix), candidate.startsWith will not match it, causing comment drafts for such issues to be incorrectly rejected. The check should also look for an exact match with the .json extension.
const candidates = await mount.listTree('/linear/issues/')
const issuePath = candidates.find((candidate) =>
candidate.startsWith(`/linear/issues/${issueKey}__`) ||
candidate === `/linear/issues/${issueKey}.json`
)
if (!issuePath) return false| const issuePath = path.includes('/comments/') ? path.split('/comments/')[0] ?? path : path | ||
| try { | ||
| const issue = parseLinearIssue(issuePath, (await mount.readFile(issuePath)).content) | ||
| return isInFactoryScope(issue, config.safety) | ||
| } catch { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Since comments are now stored under /linear/comments/ instead of /linear/issues/.../comments/..., path.startsWith('/linear/issues/') will never contain /comments/. The path.includes('/comments/') check is obsolete and can be removed to simplify the code.
try {
const issue = parseLinearIssue(path, (await mount.readFile(path)).content)
return isInFactoryScope(issue, config.safety)
} catch {
return false
}
|
No code changes were needed. I reviewed the PR diff, changed-file set, metadata, touched callers/types/tests, and the visible PR comments. I did not find a current checkout defect to patch. Addressed comments
Advisory Notes
VerificationPassed locally:
GitHub connector reports the PR is mergeable. I did not run the macOS-only |
|
Implemented fixes for the validated PR review findings. Addressed comments
Advisory NotesNone. VerificationPassed locally:
GitHub PR metadata reports |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/factory-sdk/src/__tests__/writefile-callsite-invariant.test.ts`:
- Around line 16-21: The test currently uses a fragile substring check for
"mount.writeFile(" which misses variants like "mount .writeFile(" or
newline-separated member access; update the check to parse each file's AST and
detect CallExpressions whose callee is a MemberExpression with an Identifier
named "mount" and a property Identifier "writeFile" (or equivalent computed/name
forms) instead of using source.includes('mount.writeFile('). Modify the loop in
the test (where source is read and ALLOWED_WRITEBACK_CALLSITES and offenders are
referenced) to parse the file (e.g., with `@babel/parser` or TypeScript's parser),
traverse nodes to find such member-call sites, and push rel into offenders when
a matching AST callsite is found.
In `@packages/factory-sdk/src/cli/fleet.ts`:
- Around line 285-290: The current lookup uses candidates.find(...) which can
pick the wrong match when both a canonical file and a draft/reconciled variant
exist; update the logic that computes issuePath (the result of
mount.listTree('/linear/issues/') filtered on issueKey) to reject ambiguous
matches or explicitly prefer the canonical path
`/linear/issues/${issueKey}.json`: collect all matches into an array, if there
is exactly one match use it, if multiple matches prefer the exact
`/linear/issues/${issueKey}.json` entry, otherwise return false (mirror the
uniqueness rule used by findIssuePath()) so comment draft authorization cannot
bind to the wrong parent.
In `@packages/factory-sdk/src/mount/relayfile-cloud-mount-client.ts`:
- Around line 236-253: mapOperationStatus currently throws on terminal failures
and on a succeeded response with an incomplete providerResult, which makes the
cloud MountClient behavior diverge from the MountClient contract and the testing
fakes that expect confirmWrite() to resolve to 'failed'; update
mapOperationStatus so that instead of throwing it returns the string 'failed'
for these cases (i.e., when response.status is
'failed'|'dead_lettered'|'canceled' and when response.status === 'succeeded' but
providerResult is missing/invalid as determined by
providerResultError(response)), leaving successful acked paths unchanged; ensure
confirmWrite callers and the existing fakes remain compatible.
In `@packages/factory-sdk/src/orchestrator/factory.ts`:
- Around line 31-37: The STATE_NAME_TO_ID mapping currently uses module-level
LINEAR_STATE_IDS, causing parseLinearIssue to mis-map state_name fallbacks when
a workspace overrides IDs; update parseLinearIssue (and any other parsers
referencing STATE_NAME_TO_ID) to accept and use the configured state map from
FactoryLoop (this.#config.stateIds) instead of LINEAR_STATE_IDS, thread the
config.stateIds into the parsing call sites, and replace the module-level
constant lookups with lookups against the provided config map (also update the
other identical occurrences where STATE_NAME_TO_ID is used so they use the
injected config.stateIds).
- Around line 235-239: The current catch around this.#linear.postComment in
dispatch() is swallowing all errors; change it to only downgrade the known
"unsupported comment" case to a warning and rethrow any other errors so failures
remain fatal. Specifically, inspect the thrown error from
this.#linear.postComment (e.g., instance check for a dedicated
UnsupportedCommentsError or check a unique error property/code such as
error.code === 'UNSUPPORTED_COMMENTS') and if and only if it matches, call
this.#logger.warn?.('[factory] comment writeback skipped', error); otherwise
rethrow the error to propagate failure out of dispatch().
In `@packages/factory-sdk/src/writeback/linear.ts`:
- Around line 128-131: The createIssue flow currently only asserts scope using
payload.team.key (via assertInFactoryScope(scopeIssueFromPayload(...))) but
later accepts payload.teamId alone (used when building the write payload/path),
allowing bypass; update createIssue (and the analogous block around where
payload.teamId is forwarded, e.g., lines handling payload.teamId) to reject
teamId-only payloads or validate the resolved team before writing by resolving
the team object for payload.teamId and running
assertInFactoryScope(scopeIssueFromPayload(resolvedTeamPayload, 'createIssue
payload'), safety, 'createIssue payload') (or throw if teamId cannot be
resolved), ensuring mount.writeFile is only called after the team has been
validated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c14aa7b5-b303-4c63-be18-00cd5e6554cb
📒 Files selected for processing (13)
packages/factory-sdk/src/__tests__/writefile-callsite-invariant.test.tspackages/factory-sdk/src/cli/fleet.test.tspackages/factory-sdk/src/cli/fleet.tspackages/factory-sdk/src/constants/linear.tspackages/factory-sdk/src/mount/relayfile-cloud-mount-client.test.tspackages/factory-sdk/src/mount/relayfile-cloud-mount-client.tspackages/factory-sdk/src/orchestrator/factory.test.tspackages/factory-sdk/src/orchestrator/factory.tspackages/factory-sdk/src/ports/mount.tspackages/factory-sdk/src/testing/fakes.tspackages/factory-sdk/src/writeback/linear.tspackages/factory-sdk/src/writeback/slack.tspackages/factory-sdk/src/writeback/writeback.test.ts
| for (const file of await sourceFiles(SDK_ROOT)) { | ||
| const rel = relative(SDK_ROOT, file).split('\\').join('/') | ||
| if (rel.endsWith('.test.ts') || rel.startsWith('__tests__/')) continue | ||
| const source = await readFile(file, 'utf8') | ||
| if (!source.includes('mount.writeFile(')) continue | ||
| if (!ALLOWED_WRITEBACK_CALLSITES.has(rel)) offenders.push(rel) |
There was a problem hiding this comment.
This invariant is lexical, not structural.
The exact-string scan only catches mount.writeFile( with one formatting shape. mount .writeFile( or newline-separated member access bypass this test while still adding direct write callsites outside the chokepoint. If this invariant is meant to enforce the guardrail, it should parse member-call structure instead of searching for one literal substring.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/factory-sdk/src/__tests__/writefile-callsite-invariant.test.ts`
around lines 16 - 21, The test currently uses a fragile substring check for
"mount.writeFile(" which misses variants like "mount .writeFile(" or
newline-separated member access; update the check to parse each file's AST and
detect CallExpressions whose callee is a MemberExpression with an Identifier
named "mount" and a property Identifier "writeFile" (or equivalent computed/name
forms) instead of using source.includes('mount.writeFile('). Modify the loop in
the test (where source is read and ALLOWED_WRITEBACK_CALLSITES and offenders are
referenced) to parse the file (e.g., with `@babel/parser` or TypeScript's parser),
traverse nodes to find such member-call sites, and push rel into offenders when
a matching AST callsite is found.
| const candidates = await mount.listTree('/linear/issues/') | ||
| const issuePath = candidates.find((candidate) => | ||
| candidate.startsWith(`/linear/issues/${issueKey}__`) || | ||
| candidate === `/linear/issues/${issueKey}.json` | ||
| ) | ||
| if (!issuePath) return false |
There was a problem hiding this comment.
Reject ambiguous parent issue matches before authorizing comment drafts.
This find() silently picks the first /linear/issues/${issueKey}* match. When both a canonical issue file and a draft/reconciled variant exist, the scope check can authorize the comment against the wrong parent issue. Please apply the same uniqueness rule used by findIssuePath() here, or explicitly prefer the canonical real-issue path.
Suggested fix
- const issuePath = candidates.find((candidate) =>
- candidate.startsWith(`/linear/issues/${issueKey}__`) ||
- candidate === `/linear/issues/${issueKey}.json`
- )
- if (!issuePath) return false
+ const matches = candidates.filter((candidate) =>
+ candidate.startsWith(`/linear/issues/${issueKey}__`) ||
+ candidate === `/linear/issues/${issueKey}.json`
+ )
+ if (matches.length !== 1) return false
+ const [issuePath] = matches📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const candidates = await mount.listTree('/linear/issues/') | |
| const issuePath = candidates.find((candidate) => | |
| candidate.startsWith(`/linear/issues/${issueKey}__`) || | |
| candidate === `/linear/issues/${issueKey}.json` | |
| ) | |
| if (!issuePath) return false | |
| const candidates = await mount.listTree('/linear/issues/') | |
| const matches = candidates.filter((candidate) => | |
| candidate.startsWith(`/linear/issues/${issueKey}__`) || | |
| candidate === `/linear/issues/${issueKey}.json` | |
| ) | |
| if (matches.length !== 1) return false | |
| const [issuePath] = matches |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/factory-sdk/src/cli/fleet.ts` around lines 285 - 290, The current
lookup uses candidates.find(...) which can pick the wrong match when both a
canonical file and a draft/reconciled variant exist; update the logic that
computes issuePath (the result of mount.listTree('/linear/issues/') filtered on
issueKey) to reject ambiguous matches or explicitly prefer the canonical path
`/linear/issues/${issueKey}.json`: collect all matches into an array, if there
is exactly one match use it, if multiple matches prefer the exact
`/linear/issues/${issueKey}.json` entry, otherwise return false (mirror the
uniqueness rule used by findIssuePath()) so comment draft authorization cannot
bind to the wrong parent.
| const mapOperationStatus = ( | ||
| response: OperationStatusResponse, | ||
| ): 'acked' | 'pending' | 'failed' => { | ||
| if (response.status === 'succeeded') return 'acked' | ||
| if (response.status === 'succeeded') { | ||
| const providerResult = response.providerResult | ||
| if ( | ||
| providerResult && | ||
| providerResult.status === 200 && | ||
| typeof providerResult.externalId === 'string' && | ||
| providerResult.externalId.length > 0 | ||
| ) { | ||
| return 'acked' | ||
| } | ||
| throw new Error(`Writeback provider result incomplete for ${response.path ?? response.opId}: ${providerResultError(response)}`) | ||
| } | ||
| if (response.status === 'failed' || response.status === 'dead_lettered' || response.status === 'canceled') { | ||
| return 'failed' | ||
| throw new Error(`Writeback operation failed for ${response.path ?? response.opId}: ${providerResultError(response)}`) | ||
| } |
There was a problem hiding this comment.
Keep terminal confirmWrite() failures aligned with the MountClient contract.
mapOperationStatus() now throws for failed/canceled/dead-lettered ops, but packages/factory-sdk/src/ports/mount.ts and packages/factory-sdk/src/testing/fakes.ts still model confirmWrite() as resolving 'failed'. That makes the cloud client observably incompatible with the interface and the fake, so any caller that branches on the returned status will see different behavior in tests vs. production.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/factory-sdk/src/mount/relayfile-cloud-mount-client.ts` around lines
236 - 253, mapOperationStatus currently throws on terminal failures and on a
succeeded response with an incomplete providerResult, which makes the cloud
MountClient behavior diverge from the MountClient contract and the testing fakes
that expect confirmWrite() to resolve to 'failed'; update mapOperationStatus so
that instead of throwing it returns the string 'failed' for these cases (i.e.,
when response.status is 'failed'|'dead_lettered'|'canceled' and when
response.status === 'succeeded' but providerResult is missing/invalid as
determined by providerResultError(response)), leaving successful acked paths
unchanged; ensure confirmWrite callers and the existing fakes remain compatible.
| const STATE_NAME_TO_ID: Record<string, string> = { | ||
| 'Ready for Agent': LINEAR_STATE_IDS.readyForAgent, | ||
| 'Agent Implementing': LINEAR_STATE_IDS.agentImplementing, | ||
| Implementing: LINEAR_STATE_IDS.agentImplementing, | ||
| Done: LINEAR_STATE_IDS.done, | ||
| 'In Planning': LINEAR_STATE_IDS.inPlanning, | ||
| } |
There was a problem hiding this comment.
State-name fallback ignores config.stateIds.
parseLinearIssue() now hard-codes LINEAR_STATE_IDS, but FactoryLoop decides readiness with this.#config.stateIds. If a workspace overrides the workflow IDs in config, a state_name-only record will be parsed to the built-in UUIDs and then skipped as “not ready” even though the configured ready state matches. Please thread the configured state map into parsing instead of using module-level constants here.
Also applies to: 612-613, 742-743
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/factory-sdk/src/orchestrator/factory.ts` around lines 31 - 37, The
STATE_NAME_TO_ID mapping currently uses module-level LINEAR_STATE_IDS, causing
parseLinearIssue to mis-map state_name fallbacks when a workspace overrides IDs;
update parseLinearIssue (and any other parsers referencing STATE_NAME_TO_ID) to
accept and use the configured state map from FactoryLoop (this.#config.stateIds)
instead of LINEAR_STATE_IDS, thread the config.stateIds into the parsing call
sites, and replace the module-level constant lookups with lookups against the
provided config map (also update the other identical occurrences where
STATE_NAME_TO_ID is used so they use the injected config.stateIds).
| try { | ||
| await this.#linear.postComment(issue, comment) | ||
| } catch (error) { | ||
| this.#logger.warn?.('[factory] comment writeback skipped', error) | ||
| } |
There was a problem hiding this comment.
Only downgrade unsupported comment writebacks, not every failure.
This catch now converts timeouts, guard rejections, and provider failures into warnings and still advances the Linear state. That makes dispatch() succeed after a broken writeback path, which is broader than the stated “unsupported comments are best-effort” behavior. Narrow this to the unsupported-comment case (or a dedicated error type) and keep other failures fatal.
Possible fix
try {
await this.#linear.postComment(issue, comment)
} catch (error) {
- this.#logger.warn?.('[factory] comment writeback skipped', error)
+ if (isUnsupportedCommentWriteback(error)) {
+ this.#logger.warn?.('[factory] comment writeback skipped', error)
+ } else {
+ throw error
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/factory-sdk/src/orchestrator/factory.ts` around lines 235 - 239, The
current catch around this.#linear.postComment in dispatch() is swallowing all
errors; change it to only downgrade the known "unsupported comment" case to a
warning and rethrow any other errors so failures remain fatal. Specifically,
inspect the thrown error from this.#linear.postComment (e.g., instance check for
a dedicated UnsupportedCommentsError or check a unique error property/code such
as error.code === 'UNSUPPORTED_COMMENTS') and if and only if it matches, call
this.#logger.warn?.('[factory] comment writeback skipped', error); otherwise
rethrow the error to propagate failure out of dispatch().
| async createIssue(payload: LinearCreateIssuePayload): Promise<{ path: string }> { | ||
| assertInFactoryScope(scopeIssueFromPayload(payload, 'createIssue payload'), safety, 'createIssue payload') | ||
| const path = createIssuePath(payload) | ||
| await mount.writeFile(path, payload) | ||
| await mount.writeFile(path, createIssueWritePayload(payload), { guarded: true }) |
There was a problem hiding this comment.
teamId now bypasses the factory-scope team check.
Line 129 validates scope from payload.team.key, but Lines 212-217 will still forward payload.teamId when team is absent. That lets a caller send a [factory-e2e] create payload with an arbitrary teamId, pass assertInFactoryScope(...), and create the issue outside the intended AR boundary. Reject teamId-only payloads or validate the resolved team before writing.
Possible fix
async createIssue(payload: LinearCreateIssuePayload): Promise<{ path: string }> {
+ if (typeof payload.teamId === 'string' && typeof asRecord(payload.team)?.key !== 'string') {
+ throw new Error('Linear createIssue payload must include team.key when teamId is provided')
+ }
assertInFactoryScope(scopeIssueFromPayload(payload, 'createIssue payload'), safety, 'createIssue payload')
const path = createIssuePath(payload)
await mount.writeFile(path, createIssueWritePayload(payload), { guarded: true })Also applies to: 212-217
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/factory-sdk/src/writeback/linear.ts` around lines 128 - 131, The
createIssue flow currently only asserts scope using payload.team.key (via
assertInFactoryScope(scopeIssueFromPayload(...))) but later accepts
payload.teamId alone (used when building the write payload/path), allowing
bypass; update createIssue (and the analogous block around where payload.teamId
is forwarded, e.g., lines handling payload.teamId) to reject teamId-only
payloads or validate the resolved team before writing by resolving the team
object for payload.teamId and running
assertInFactoryScope(scopeIssueFromPayload(resolvedTeamPayload, 'createIssue
payload'), safety, 'createIssue payload') (or throw if teamId cannot be
resolved), ensuring mount.writeFile is only called after the team has been
validated.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/factory-sdk/src/orchestrator/factory.ts (1)
638-645:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate that
payload.urlreconciles to the same Linear issue.
isRealLinearIssue()only checks thatidentifierlooks likeAR-123and thatpayload.urlis non-empty. That means a synthetic or stale record withidentifier: 'AR-123'and any arbitrary URL still passes the new real-issue gate, sorunOnce(),dispatch(), and#handleChange()can still act on unreconciled drafts. Parse the URL and require it to point at the same Linear issue key before returningtrue.Possible fix
const isRealLinearIssue = (issue: LinearIssue): boolean => { const payload = wrappedPayload(issue.raw) const identifier = stringValue(payload.identifier) ?? issue.key + const url = stringValue(payload.url) + if (!url) return false + + let urlIssueKey: string | undefined + try { + const parts = new URL(url).pathname.split('/').filter(Boolean) + const issueIndex = parts.indexOf('issue') + urlIssueKey = issueIndex >= 0 ? parts[issueIndex + 1]?.toUpperCase() : undefined + } catch { + return false + } + return identifier === issue.key && /^[A-Z]+-\d+$/u.test(identifier) && - typeof payload.url === 'string' && - payload.url.length > 0 + urlIssueKey === identifier }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/factory-sdk/src/orchestrator/factory.ts` around lines 638 - 645, isRealLinearIssue currently only checks that payload.url is a non-empty string; update it to parse payload.url (using the URL constructor inside a try/catch) and extract the Linear issue key from the URL path or fragment (the part that contains the issue identifier), then require that extracted key equals issue.key in addition to the existing identifier checks; keep using wrappedPayload(issue.raw) and stringValue(payload.identifier) but return false if URL parsing fails or the URL's issue key doesn't match issue.key so only reconciled Linear links pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/factory-sdk/src/orchestrator/factory.ts`:
- Around line 638-645: isRealLinearIssue currently only checks that payload.url
is a non-empty string; update it to parse payload.url (using the URL constructor
inside a try/catch) and extract the Linear issue key from the URL path or
fragment (the part that contains the issue identifier), then require that
extracted key equals issue.key in addition to the existing identifier checks;
keep using wrappedPayload(issue.raw) and stringValue(payload.identifier) but
return false if URL parsing fails or the URL's issue key doesn't match issue.key
so only reconciled Linear links pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fec7ad9b-057e-4a5e-9dc7-de4018875f4c
📒 Files selected for processing (5)
packages/factory-sdk/src/mount/relayfile-cloud-mount-client.tspackages/factory-sdk/src/orchestrator/factory.tspackages/factory-sdk/src/ports/mount.tspackages/factory-sdk/src/writeback/linear.tspackages/factory-sdk/src/writeback/writeback.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/factory-sdk/src/ports/mount.ts
…letes (D7, #244) deleteFile default-deny on provider paths (/linear/**,/slack/**); allow ONLY a this-client unreconciled orphan draft (tracked op terminally failed/dead_lettered/canceled + no providerResult.externalId + no url/real-key + injected isAllowedDelete) — race/unknown-op/succeeded-with-externalId all REFUSE. Structural invariant extended to mount.deleteFile. Reviewed @a24c484 (w9-review V0, all clauses mutation-checked; post-rebase verified no resurrection of the pre-#243 fail-open) + factory-verify live V1 GREEN (mount.deleteFile(AR-133)→REFUSED at the linked check, canary intact).
1 participant
Summary
/linear/comments/....getOp(opId)terminal provider-push status: ACK only onop.status === "succeeded"plusproviderResult.status === 200andexternalId.mount.writeFilecallsite invariant, and the cloud-client draft chokepoint with an async injected predicate for markerless setState/comment writes.Tests
npx vitest run packages/factory-sdknpx tsc --noEmit -p tsconfig.node.jsonNotes
src/mainedits.