Repository navigation
feat(workflow): record team revision - #1201
ousamabenyounes wants to merge 1 commit into
Conversation
|
I can’t complete the exact-scope review: the checkout is shallow, and the requested The supplied PR description meets the testing-record requirement: it documents a build and representative real-CLI run with published provenance. Please provide the exact three-dot diff or a checkout containing the missing base history. No PR code was run, built, or installed. |
SaulMoro
left a comment
SaulMoro
left a comment
There was a problem hiding this comment.
Checked against #1163 and its follow-up comment on the full commit id.
| Spec item | Covered by | Status |
|---|---|---|
1. cat-file -e resolves every published value |
e2e, in the producing clone (see inline) | met |
2. git show + hash equals sourceHash |
e2e, and the real-CLI run in the body | met |
3. uncommitted or unpushed edit publishes no teamRevision |
stores only a remote-reachable commit that holds the cached workflow bytes |
met |
| 4. kept entry keeps its earlier value | mixed parent/child test asserts the kept cache | met in code; no test runs a kept single workflow and reads the run's value |
5. parent and child at different commits give no teamRevision |
mixed parent/child test | met |
| 6. legacy records parse and merge; new fields pass the current reader | records test; the reader is JSON.parse plus a spreading mergeRecord |
met |
| Comment: full 40-char commit | rev-parse HEAD |
met |
| Watch-outs: no fetch in the cache step, both guide pages, based on #1148 | met |
Limiting reachability to refs/remotes/origin/* is narrower than the issue's "a remote-tracking ref". It is the right reading: origin is the team repo, so criterion 1 holds by construction and a personal remote cannot make a commit publishable. Clearing a kept revision that origin no longer reaches goes past the issue text and stays within its intent.
Ran npx vitest run src/__tests__/workflow-cache.test.ts src/__tests__/workflow-records.test.ts on a248c4d: 199 passed.
One non-blocking finding and two nits inline. Approving.
| if (options.dryRun || (previous?.sourceHash === file.sourceHash && previous.teamRevision === file.teamRevision)) continue; | ||
| await writeJsonAtomic(cachePath(config, file.definition.name), { | ||
| relativePath: file.relativePath, | ||
| sourceHash: file.sourceHash, | ||
| ...(file.teamRevision ? { teamRevision: file.teamRevision } : {}), |
There was a problem hiding this comment.
[P2 non-blocking] An unrelated unpushed commit erases a revision that is still valid.
Scenario: build.yaml is pushed at A and cached with teamRevision: A. The member then commits something unrelated and does not push. This is the normal state in self mode, where .teamai/ lives in the project repo and HEAD is usually ahead of origin. On the next pull or push, reachable is undefined, so file.teamRevision is undefined. It differs from previous.teamRevision, so the entry is rewritten without A. The run then publishes no teamRevision, although A still holds those exact bytes and origin still reaches it.
An equal sourceHash means equal bytes. The kept loop above has already removed a previous revision that origin no longer reaches. So the earlier value can be carried forward, which is the issue's rule for kept entries applied to entries confirmed unchanged:
| if (options.dryRun || (previous?.sourceHash === file.sourceHash && previous.teamRevision === file.teamRevision)) continue; | |
| await writeJsonAtomic(cachePath(config, file.definition.name), { | |
| relativePath: file.relativePath, | |
| sourceHash: file.sourceHash, | |
| ...(file.teamRevision ? { teamRevision: file.teamRevision } : {}), | |
| const teamRevision = file.teamRevision ?? (previous?.sourceHash === file.sourceHash ? previous.teamRevision : undefined); | |
| if (options.dryRun || (previous?.sourceHash === file.sourceHash && previous.teamRevision === teamRevision)) continue; | |
| await writeJsonAtomic(cachePath(config, file.definition.name), { | |
| relativePath: file.relativePath, | |
| sourceHash: file.sourceHash, | |
| ...(teamRevision ? { teamRevision } : {}), |
Regression test. It fails on a248c4d. With the change it passes, both test files stay green (200 passed), and tsc --noEmit is clean:
it('keeps a verified revision when an unrelated unpushed commit moves HEAD', async () => {
const config = testConfig();
const repository = initializeTeamRepository(config);
writeWorkflow(config, 'build', VALID);
const pushed = repository.commit('add workflow');
repository.push();
await syncWorkflowDefinitions(config);
fs.writeFileSync(path.join(config.repo.localPath, 'README.md'), 'unrelated\n');
repository.commit('unrelated local commit');
await syncWorkflowDefinitions(config);
expect((await loadCachedWorkflow(config, 'build'))?.teamRevision).toBe(pushed);
});| expect(startRecord.username).toBe('alice'); | ||
| expect(startRecord.sourceHash).toBe(sourceHash); | ||
| expect(startRecord.teamRevision).toBe(teamRevision); | ||
| expect(() => git(repo, 'cat-file', '-e', teamRevision)).not.toThrow(); |
There was a problem hiding this comment.
[P3 nit] Criterion 1 says "in a fresh clone". repo is the clone that produced the value, so cat-file -e passes there whether or not origin has the commit. Checking criteria 1 and 2 against a fresh clone of origin.git proves the claim:
| expect(() => git(repo, 'cat-file', '-e', teamRevision)).not.toThrow(); | |
| git(sandbox, 'clone', 'origin.git', 'fresh'); | |
| const fresh = path.join(sandbox, 'fresh'); | |
| expect(() => git(fresh, 'cat-file', '-e', teamRevision)).not.toThrow(); | |
| expect(git(fresh, 'show', `${teamRevision}:workflows/build.yaml`)).toBe(TWO_STEPS); |
If the real-CLI check in the PR body was also run in the working clone, a fresh clone there would match the issue's test plan.
| async function remotelyReachableRevision(root: string): Promise<ReachableRevision | undefined> { | ||
| try { | ||
| const git = createGit(root); | ||
| const revision = (await git.revparse(['HEAD'])).trim(); |
There was a problem hiding this comment.
[P3 nit] The value is already the full commit id, so the issue comment is satisfied. The comment points at getHeadCommit (utils/git.ts), which #1176 and #1180 use for the same purpose. Reusing it keeps one way to resolve the team commit:
const revision = await getHeadCommit(root);
if (!revision || !await isTeamRemoteRevision(root, revision)) return undefined;3 participants
Summary
Workflow run records currently expose only a rendered-source hash, so a reviewer cannot tie a run to the exact shared team commit that supplied it. Capture the team
origincommit only when it stores the exact cached workflow and prompt bytes, propagate it when the whole workflow tree agrees, and publish it besidesourceHashin start/end records.Keep legacy records readable while documenting the provenance fields in English and Chinese.
Type of Change
Test Plan
npx tsc --noEmitpassesnpm run lintpassesnpx vitest runpasses in exact-head fork CI; the local host's baseline-identical failures are documented belowTest verification (RED → GREEN)
On unmodified upstream, the targeted behavioral expectations fail for missing cache/run provenance. For example:
The two negative-origin cases each include a positive
origincontrol. On unmodified upstream, those controls fail first because provenance does not exist yet:Making the reader type require current provenance also proves the legacy compatibility problem:
After the implementation, the complete focused files pass, including the
cross-platform symlink-alias regression:
The first exact-head fork CI exposed a macOS-only
/var→/private/varcanonical-path mismatch. The new regression failed before the fix:
Canonicalizing the Git top level and source path makes that test pass. Removing
only those two
realpathcalls returns the same assertion to RED; restoringthem returns all 169 workflow-cache tests to GREEN.
The changed reports-branch E2E passes separately:
Mutation checks restore the exact final production files, make the reader require current provenance, and return an empty legacy queue. Restoring the four production files on the final test commit returns every provenance expectation to RED; the output identifies the failing assertion and location for each case:
A selective mutant that accepts any checked-out commit and skips source-byte comparison reaches the two negative assertions themselves:
The strict-reader mutation fails compilation:
The empty-reader mutation fails the real queue assertion:
Production diff coverage is complete:
Exact-head CI for
a248c4def0e76d53438f877c54d736c3a933509eis green onUbuntu and macOS with Node 20, 22, and 24; the build plus its focused E2E gates
also pass: https://github.com/ousamabenyounes/teamai-cli/actions/runs/39093552069.
Full local suite
run-ci.shran on both the unmodified base and final head. Install, lint, typecheck, build, targeted OpenCode recall, and unattended-init gates pass on final. The baseline generic diff gate exits 1 because no baseline coverage file exists; the final generic gate exits 1 because it classifies changed TypeScript test files as production while LCOV omits tests. The production-only final diff result above is 80/80:Both unit runs include the Vitest runner header. Unit coverage has the same six host-local failures and 144 skips, with nine new passes:
This host initially lacked
sqlite3and had Git 2.43 while the config-hook path requires Git 2.54; supplying isolated SQLite 3.45.1 and Git 2.54 makes five of those six tests pass. The remaining unrelated uninstall assertion reproduces on the unmodified base. Upstream CI for base SHAf58f3154e2d003d2da1fcd7e6e9223cde107f38dis green across the complete Ubuntu/macOS Node 20/22/24 unit matrix: https://github.com/Tencent/teamai-cli/actions/runs/39039041492. The PR CI matrix is the final authority for that host-specific assertion.Extracting the failed test names from the baseline and final logs yields the same set:
Fork E2E retains the same pre-existing
submodule-checkout-1041hook-config failure. Baseline and amended final SHA both have 1,248 passed / 1 failed / 64 skipped. The targeted OpenCode gate also passes.Real CLI verification
The built CLI ran
teamai pullandteamai workflow start verify --driver local; the run published successfully:The published record contains the expected provenance:
git cat-file -t <teamRevision>returnscommit, and hashinggit show <teamRevision>:.teamai/workflows/verify.yamlreproducessourceHash:Related Issues
Closes #1163
Notes for Reviewers
The configured team repository is the
originremote, so personal remotes cannot make a local commit publishable. No fetch or other network operation occurs while caching.teamRevisionstays optional for legacy readers. A non-code validation failure may retain an earlier valid cache and its still-reachable revision; coded diagnostics continue to discard that cache under the existing safety rule.