docs(openspec): propose a runtime demo path - #235
Conversation
|
@claude /review |
|
Claude finished @thecodedrift's task in 5m 19s —— View job Review completeReview mode: incremental — read 0 prior review thread(s) before reviewing.
This is a proposal-only change (no implementation), so the review focused on internal consistency of the OpenSpec artifacts and whether their claims hold up against the current codebase. Posted 3 inline comments:
Minor, not inlined:
No blocking issues beyond the internal-consistency points above — the surrounding design reasoning (stopping at |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
a704a09 to
31a04a0
Compare
Addressed in 31a04a0. Taking the three findings in order: 1. 2. The 3. Both minor items also taken. Task 0.2 now separates "can the service serve this unauthenticated" (open) from what the demo does either way (settled by D3 — it stops at Grepped the whole change directory afterwards for — AI Coding Agent |
Review feedback on #235. D1 claimed `submitRule`, `pollRuleStatus` and `writeRuleFile` are "reused, not reimplemented", and for two of the three that is not true yet. Both requesters are hardcoded to the rule-family endpoints and take a required `token`, which `createApiClient` turns unconditionally into an `Authorization: Bearer` header. The demo is unauthenticated by requirement, so neither is usable as written. The path half resolves itself when the client adopts the renamed family; the token half does not, and is the durable part. The reason this is worth more than the wording: D1's failure test says "if the demo needs a new writer, the shapes have diverged, and the demo has done its job by failing". That test is a claim about PAYLOAD SHAPES, and it was already tripping — for the requester and the poller, where it means nothing of the kind. A requester needing a second argument says nothing about whether the demo and the mainline agree on what a rule looks like. So D1 now splits them. `writeRuleFile` is byte-for-byte reuse and is what the failure test is really about. The other two are two small changes to functions written when every caller was authenticated, and they are planned as task 1.2a rather than discovered mid-implementation, which is what the previous wording set up. 1.2b is the part nobody raised: an unauthenticated call must send no `Authorization` header rather than one carrying an empty token. A header the service then has to decide how to ignore is a worse contract than its absence.
The runtime tier cannot be demonstrated or smoke-tested end to end. Everything we can exercise alone covers the pieces; nothing covers the two things that exist only between this client and the service, which are generation producing a runtime rule and delivery handing it over in a shape we accept. Every defect the alignment work surfaced lived in that seam and was found by someone noticing. The shape, as asked of the generator team in N9: two endpoints mirroring the mainline, a ticket then a retrieval, returning the published file-set variant. Mirroring is the decision rather than a convenience — a bespoke demo payload would exercise a path no user is on, which is the one thing a demo must not do, and reusing the well-known formats means the demo covers polling and the terminal states because it is the same client code reaching them. Two things the design writes down because they are the tempting ones to get wrong. The demo stops at `verify`, and says so. A runtime rule executes only when an authenticated reconcile returns its signature, and the demo is unauthenticated, so it reaches a complete verified rule on disk and no further. A client-side bypass keyed on the demo rule id would show the real thing and is refused: the signature gate is the only thing between a payload fetched over the network and arbitrary code running on a developer's machine, and the id it would key on is visible to anyone who reads the recipe. The spec carries that as a requirement rather than a comment. Hiding the topic needs no mechanism. `RECIPE_TOPICS` is a hand-maintained literal and lookup is by filename, so a recipe absent from the list is already fetchable by name and invisible to the index. `cli-agent` gains a requirement saying that is intended, so nobody later reads it as an oversight and closes it. Tasks are blocked on the endpoints existing. Building against a guessed shape is what produced the seam this change is closing.
The generator team is renaming the request resource to `request`/`requestId` as its own change, ahead of this one. Their `openspec/specs/cli/spec.md` already specified that noun, and their live `meta` block already carries `ticketId` per delivered rule, so the route was the outlier rather than the convention. D2 said the demo must never be where a second convention lives. That was a principle; the rename makes it a sequencing constraint. Naming these endpoints today would make the demo the debut of the new noun, which is the thing D2 rules out. Task 0.0 waits for the rename and the regenerated schema, and nothing starts before it. Also corrects a premise the generator team refuted. This proposal said generation runs against "a public repository the service controls". It does not: CLI-bound generation never clones, the clone is deferred to the git-bound push that only happens for a pull request, and `repositoryUrl` on a CLI request is authorization scoping rather than model input. The fixed input is a prompt and its examples held as data. Recorded as D2a, because the wrong version was specific enough to be believed, and a later reader would have tried to make the repository load-bearing.
Task 0.0 waited for `request`/`requestId` to become canonical. It has:
verified from `GET /cli/api/__schema` that `/cli/api/request/*` is
canonical and the `rule/*` family is `deprecated: true` while still
serving.
So the demo endpoints are `POST /cli/api/demo/request` and
`GET /cli/api/demo/request/{requestId}`. That was the whole reason for
holding this change behind the rename: naming them earlier would have
made the demo the place a new convention debuted, which is what D2 rules
out. The mainline took the noun first and the demo follows it.
Adopting the renamed paths in the ordinary client is its own change and
is deliberately not a dependency of this one. The legacy family still
serves, so the two can land in either order.
The previous commit renamed the endpoints in the proposal and the task list header and left two references behind, in task 0.1 and in design D1. Both still said `demo/rule` and `ruleId`. Recording it rather than folding it in silently, because it is the exact failure this change is about: the noun moved in one place and not in the places describing it, and nothing measured the gap. Grepped for the old spelling afterwards this time. 0.1 is also checked off — the shapes are agreed on both sides.
D2 still framed the request rename as pending while D1 and tasks 0.0/0.0a already recorded it as landed, so a reader met a contradiction about which naming is current. D2 now states the principle, then the rename as history, and the Risks entry describes the skew that actually remains: the demo is born on request/requestId while the ordinary client stays on the deprecated family. The cli-agent requirement also only covered RECIPE_TOPICS, but `taskless agent` prints two independently-sourced listings and the other one iterates the top-level command tree, filtering only `agent` itself. Nothing said where the demo command lives, so a top-level verb would have been hidden as a recipe and advertised as a command in the same output. D4a settles it as `rule demo`, nested under the existing rule command, obtaining the property by not registering rather than by teaching a renderer to skip.
Review feedback on #235. D1 claimed `submitRule`, `pollRuleStatus` and `writeRuleFile` are "reused, not reimplemented", and for two of the three that is not true yet. Both requesters are hardcoded to the rule-family endpoints and take a required `token`, which `createApiClient` turns unconditionally into an `Authorization: Bearer` header. The demo is unauthenticated by requirement, so neither is usable as written. The path half resolves itself when the client adopts the renamed family; the token half does not, and is the durable part. The reason this is worth more than the wording: D1's failure test says "if the demo needs a new writer, the shapes have diverged, and the demo has done its job by failing". That test is a claim about PAYLOAD SHAPES, and it was already tripping — for the requester and the poller, where it means nothing of the kind. A requester needing a second argument says nothing about whether the demo and the mainline agree on what a rule looks like. So D1 now splits them. `writeRuleFile` is byte-for-byte reuse and is what the failure test is really about. The other two are two small changes to functions written when every caller was authenticated, and they are planned as task 1.2a rather than discovered mid-implementation, which is what the previous wording set up. 1.2b is the part nobody raised: an unauthenticated call must send no `Authorization` header rather than one carrying an empty token. A header the service then has to decide how to ignore is a worse contract than its absence.
Review feedback on #235. D1 claimed `submitRule`, `pollRuleStatus` and `writeRuleFile` are "reused, not reimplemented", and for two of the three that is not true yet. Both requesters are hardcoded to the rule-family endpoints and take a required `token`, which `createApiClient` turns unconditionally into an `Authorization: Bearer` header. The demo is unauthenticated by requirement, so neither is usable as written. The path half resolves itself when the client adopts the renamed family; the token half does not, and is the durable part. The reason this is worth more than the wording: D1's failure test says "if the demo needs a new writer, the shapes have diverged, and the demo has done its job by failing". That test is a claim about PAYLOAD SHAPES, and it was already tripping — for the requester and the poller, where it means nothing of the kind. A requester needing a second argument says nothing about whether the demo and the mainline agree on what a rule looks like. So D1 now splits them. `writeRuleFile` is byte-for-byte reuse and is what the failure test is really about. The other two are two small changes to functions written when every caller was authenticated, and they are planned as task 1.2a rather than discovered mid-implementation, which is what the previous wording set up. 1.2b is the part nobody raised: an unauthenticated call must send no `Authorization` header rather than one carrying an empty token. A header the service then has to decide how to ignore is a worse contract than its absence.
4fd79ff to
4e6b068
Compare
The generator team settled the demonstration as account-free: no ticket, no corpus entry, no billing, no authentication, one Taskless-owned installation behind every request. Three things follow. The endpoints take no auth at all rather than an ignorable empty token, so the header ask is a property instead of a convention. Retrieval also serves what the check found against the fixture's examples, which the verification gate was already computing and discarding. D3 said logging in was "the next beat" to reach a blessed execution. That is wrong: blessing is recording, and a fixed rule shared by every caller is never recorded for any of them, so authenticating reaches nothing. The gate is unreachable by construction from a shared fixture rather than one step away. Exercising the handshake needs a rule recorded for the org that runs it, which is an integration test in an org we own, not a demo. D6 takes the findings as `Finding[]` verbatim, grouped per example with what that example was expected to do. `Finding` is our own runtime-rule type, so a narrower demo-only object would be the bespoke shape D1 forbids. The grouping is what carries the demonstration: a flat list renders identically for a rule that catches the failing examples and one that fires on everything, and the second is this area's recurring defect wearing a success costume. D7 states what the negative framing obscures. The rule is real and inspectable; it is inert because no signature for it is in any org's run set, which is the gate working on the ordinary path; it is removable through the delete flow we own; and improve refuses it, correctly, because nothing pre-generated can match a rule authored against the developer's own repo. Tasks 3.4 and 5.2 pin the two that would fail silently.
4e6b068 to
b3f5e50
Compare
|
@claude /review |
|
Claude finished @thecodedrift's task in 5m 20s —— View job Review completeReview mode: incremental — read 3 prior review thread(s) before reviewing. All 3 are resolved with
I re-verified all three against the current files and current source ( 3 new findings, posted inline:
No other blocking issues — the rest of the internal-consistency and codebase cross-checks (D3/D4/D4a placement claims, |
Review of the assembled change found three places where the documents assert
something that is not true, or not yet agreed.
**The skew premise was stale.** D1, D2 and the Risks entry described the
`request`/`requestId` rename as landed on the service and pending in our
client. It landed in our client too: `submitRule` posts to `/cli/api/request`,
`pollRuleStatus` gets `/cli/api/request/{requestId}`, `iterateRule` and
`restoreRule` follow, and no hand-written call site targets the `rule/*`
family. The reasoning stays and moves to past tense, and the Risks entry now
records what the move taught: the types caught none of it, because a deprecated
path is still in the OpenAPI document and type-checks as well as the canonical
one. A vendored document and `api-deprecated-paths.test.ts` are what close that
gap. D1's "the path half resolves itself" is corrected in place - it did not,
and the literal being a literal was always the real obstacle.
**The proposal overclaimed `examples[]`.** Its retrieval code block showed
`{ requestId, status, rules[], examples[] }` as settled while D1 and task 0.1
say `rules[]` and task 0.4 is open. The block is back to the agreed shape, with
a sentence saying the findings payload is an open ask. D6 keeps its reasoning
and gains a note that its shape is under revision.
**`RULE_NOT_FOUND` on `improve` was assumed.** `iterateRule` maps a 404
`request_not_found` to `CLIError(RULE_NOT_FOUND)` but a 403 `access_denied` to
a plain `Error`, which `improveCommand` reports as `NETWORK_ERROR`. The demo
request exists under a Taskless-owned installation and is not the caller's,
which is 403-shaped. D7 and the spec requirement now state the behaviour -
improvement refused, nothing written, and a terminal failure rather than one
inviting a retry - without asserting a code in either direction. New task 0.5
asks the service which status a foreign-org ticket returns; task 3.4 pins the
behaviour now and the code after.
All three addressed in 76b3952, and all three were correct. Replies are on the inline threads; the short version: 1. The stale skew premise — verified, then rewritten to past tense. 2. The 3. Checks: — AI Coding Agent |
Review feedback on #235. D1 claimed `submitRule`, `pollRuleStatus` and `writeRuleFile` are "reused, not reimplemented", and for two of the three that is not true yet. Both requesters are hardcoded to the rule-family endpoints and take a required `token`, which `createApiClient` turns unconditionally into an `Authorization: Bearer` header. The demo is unauthenticated by requirement, so neither is usable as written. The path half resolves itself when the client adopts the renamed family; the token half does not, and is the durable part. The reason this is worth more than the wording: D1's failure test says "if the demo needs a new writer, the shapes have diverged, and the demo has done its job by failing". That test is a claim about PAYLOAD SHAPES, and it was already tripping — for the requester and the poller, where it means nothing of the kind. A requester needing a second argument says nothing about whether the demo and the mainline agree on what a rule looks like. So D1 now splits them. `writeRuleFile` is byte-for-byte reuse and is what the failure test is really about. The other two are two small changes to functions written when every caller was authenticated, and they are planned as task 1.2a rather than discovered mid-implementation, which is what the previous wording set up. 1.2b is the part nobody raised: an unauthenticated call must send no `Authorization` header rather than one carrying an empty token. A header the service then has to decide how to ignore is a worse contract than its absence.
1 participant
Proposal only — no implementation, and the tasks are deliberately blocked on the service half.
Why
The runtime tier cannot be demonstrated, and it cannot be smoke-tested end to end. Discovery, signing, validation and writing are ours; generation is the service's. The handshake between them belongs to neither and is tested by neither.
That is not abstract. Every defect the payload alignment work surfaced lived in that seam and was found by a person noticing: a runtime rule below the file-set floor admitted rather than withheld, a signature attached to the wrong rule, a capture dropped for want of a name, a rule written and never executed. An empty scan reports success, so the symptom was almost always "no findings" rather than an error.
The shape
Two endpoints mirroring the mainline, asked of the generator team as N9:
Mirroring is the decision, not a convenience. A bespoke demo payload would exercise a path no user is on, which is the one thing a demo must not do. Reusing the well-known formats means
submitRule,pollRuleStatusandwriteRuleFileare reused rather than reimplemented — so the demo covers polling and the terminalfailed/unsupportedstates for free, because it is the same client code reaching them. If the demo ever needs its own writer, that is evidence the shapes have diverged, and the demo has done its job by failing.Two things written down because they are the tempting ones to get wrong
It stops at
verify, and says so. A runtime rule executes only when an authenticated reconcile returns its signature inrun. The demo is unauthenticated, so it reaches a complete, verified rule on disk and no further —checkalready explains that state.Three options existed. Stopping at
verifyis honest.--dangerously-run-scriptsreaches execution under the warning it always prints. A client-side bypass keyed on the demo rule id is refused, and the spec carries that as a requirement rather than a comment: the signature gate is the only thing between a payload fetched over the network and arbitrary code running on a developer's machine, and the identifier it would key on is visible to anyone who reads the recipe.Hiding the topic needs no mechanism.
RECIPE_TOPICSincommands/agent.tsis a hand-maintained literal andgetRecipelooks up by filename, so a recipe absent from the list is already fetchable by name and invisible to the index.cli-agentgains a requirement stating that is intended, so a later reader does not treat it as an oversight and close it.And the command nests rather than registering.
taskless agentprints two independently-sourced listings, and only the second isRECIPE_TOPICS; the first iterates the top-level command tree and filters justagentitself. A new top-level verb would therefore be hidden as a recipe and advertised as a command in the same output. The demo isrule demo, a subcommand of the existingrulecommand, soSUBCOMMAND_NAMESandsubCommandsare untouched and no filtering code has to be written (design D4a).Deliberately blocked
Task group 0 is the service half. Nothing below it starts until the endpoint shapes are confirmed, because building against a guessed shape is what produced the seam this change exists to close.
N10 has since landed.
ruleIdnamed a ticket rather than a rule — by the service's own field description, "The ticket id the rules were generated under." The generator team renamed the resource torequest/requestId, andGET /cli/api/__schemanow shows/cli/api/request/*as canonical with therule/*familydeprecated: truewhile still serving. The demo takes the settled noun, which is why the paths above readrequest. Adopting it in the ordinary client is a separate change and is not this one's dependency. The demo must never be where a second convention debuts.Verification
openspec validate runtime-demo-pathpasses, 4/4 artifacts complete. Lint andpnpm cli checkclean (4 pre-existingno-hedgingwarnings inroute.txt/onboard.txt, untouched by this change).